diff --git a/.github/workflows/platform-vitest-main.yaml b/.github/workflows/platform-vitest-main.yaml index ca63d9323f..46c3523044 100644 --- a/.github/workflows/platform-vitest-main.yaml +++ b/.github/workflows/platform-vitest-main.yaml @@ -100,7 +100,7 @@ jobs: - name: Install macOS test dependencies run: | set -euo pipefail - brew install bash coreutils gawk ripgrep + brew install bash coreutils fd gawk ripgrep printf '%s\n' \ "$(brew --prefix bash)/bin" \ "$(brew --prefix coreutils)/libexec/gnubin" \ diff --git a/src/lib/shields/flow.test.ts b/src/lib/shields/flow.test.ts index 63212fbaaf..42c74b9527 100644 --- a/src/lib/shields/flow.test.ts +++ b/src/lib/shields/flow.test.ts @@ -15,12 +15,15 @@ const HUNG_FORWARD_OWNER_SOURCE = ` const { spawn } = require("node:child_process"); const childScriptPath = process.argv[2]; const sentinelPath = process.argv[3]; -spawn(process.execPath, [childScriptPath, sentinelPath], { stdio: "ignore" }); +const childReadyPath = process.argv[4]; +spawn(process.execPath, [childScriptPath, sentinelPath, childReadyPath], { stdio: "ignore" }); setTimeout(() => {}, 5000); `; const LATE_WEAKENING_CHILD_SOURCE = ` const fs = require("node:fs"); const sentinelPath = process.argv[2]; +const childReadyPath = process.argv[3]; +fs.writeFileSync(childReadyPath, String(process.pid)); setTimeout(() => fs.writeFileSync(sentinelPath, "ran"), 1200); setTimeout(() => {}, 5000); `; @@ -350,7 +353,7 @@ describe("shields command flow", () => { const shields = requireDist(shieldsModulePath) as { excludeRecoveryProcessTree: ( descendants: Array<{ pid: number; startIdentity: string; depth: number }>, - recoveryPid: number, + recovery: { pid: number; startIdentity: string }, recoveryDescendants: Array<{ pid: number; startIdentity: string; depth: number }>, ) => Array<{ pid: number; startIdentity: string; depth: number }>; }; @@ -359,12 +362,29 @@ describe("shields command flow", () => { const weakeningChild = { pid: 300, startIdentity: "policy-set", depth: 1 }; expect( - shields.excludeRecoveryProcessTree([recovery, recoveryChild, weakeningChild], recovery.pid, [ + shields.excludeRecoveryProcessTree([recovery, recoveryChild, weakeningChild], recovery, [ recoveryChild, ]), ).toEqual([weakeningChild]); }); + it("does not exclude a weakening child that reused a recovery PID", () => { + const shields = requireDist(shieldsModulePath) as { + excludeRecoveryProcessTree: ( + descendants: Array<{ pid: number; startIdentity: string; depth: number }>, + recovery: { pid: number; startIdentity: string }, + recoveryDescendants: Array<{ pid: number; startIdentity: string; depth: number }>, + ) => Array<{ pid: number; startIdentity: string; depth: number }>; + }; + const recovery = { pid: 200, startIdentity: "timer", depth: 1 }; + const sampledRecoveryChild = { pid: 201, startIdentity: "timer-child", depth: 2 }; + const reusedPidChild = { pid: 201, startIdentity: "policy-set", depth: 1 }; + + expect( + shields.excludeRecoveryProcessTree([reusedPidChild], recovery, [sampledRecoveryChild]), + ).toEqual([reusedPidChild]); + }); + it("auto-restore waits for the forward shields-down commit before reclaiming policy", () => { const harness = createHarness(); const stateDir = path.join(tmpDir, ".nemoclaw", "state"); @@ -441,14 +461,16 @@ describe("shields command flow", () => { ); }); - it("preempts a hung forward owner and its weakening subprocess before restoring", async () => { - const harness = createHarness(); + it("preempts a hung forward owner and its weakening subprocess before restoring", { + timeout: 10_000, + }, async () => { const stateDir = path.join(tmpDir, ".nemoclaw", "state"); fs.mkdirSync(stateDir, { recursive: true }); const sandboxName = "openclaw"; const processToken = "b".repeat(32); const snapshotPath = path.join(stateDir, "policy-snapshot-hung.yaml"); const sentinelPath = path.join(stateDir, "late-weakening-child-ran"); + const childReadyPath = path.join(stateDir, "late-weakening-child-ready"); const transitionPath = path.join( stateDir, `shields-transition-${sandboxName}-${processToken}.json`, @@ -469,13 +491,51 @@ describe("shields command flow", () => { }), ); - const owner = spawn(process.execPath, [ownerScriptPath, childScriptPath, sentinelPath], { - stdio: "ignore", - }); + const owner = spawn( + process.execPath, + [ownerScriptPath, childScriptPath, sentinelPath, childReadyPath], + { stdio: "ignore" }, + ); expect(owner.pid).toBeTypeOf("number"); + await vi.waitFor(() => expect(fs.existsSync(childReadyPath)).toBe(true), { + timeout: 5_000, + interval: 10, + }); + const childPid = Number(fs.readFileSync(childReadyPath, "utf-8")); + expect(Number.isInteger(childPid) && childPid > 0).toBe(true); const timerControl = requireDist("./timer-control.js"); const ownerStartIdentity = timerControl.readProcessStartIdentity(owner.pid); expect(ownerStartIdentity).toBeTypeOf("string"); + const childStartIdentity = timerControl.readProcessStartIdentity(childPid); + expect(childStartIdentity).toBeTypeOf("string"); + const initialDescendants = timerControl.listDescendantProcessIdentities(owner.pid); + expect(initialDescendants).not.toBeNull(); + expect(initialDescendants.some(({ pid }: { pid: number }) => pid === childPid)).toBe(true); + const takeoverEvents: string[] = []; + const readProcessStartIdentity = timerControl.readProcessStartIdentity; + let unreadableOwnerIdentityReads = 2; + vi.spyOn(timerControl, "readProcessStartIdentity").mockImplementation((...args: unknown[]) => { + const [pid, deadline] = args as [number, number?]; + const unreadable = pid === owner.pid && unreadableOwnerIdentityReads > 0; + unreadableOwnerIdentityReads -= unreadable ? 1 : 0; + return unreadable ? null : readProcessStartIdentity(pid, deadline); + }); + const readProcessState = timerControl.readProcessState; + vi.spyOn(timerControl, "readProcessState").mockImplementation((...args: unknown[]) => { + const [pid, deadline] = args as [number, number?]; + const state = readProcessState(pid, deadline); + pid === owner.pid && /^[Tt]/.test(state ?? "") && takeoverEvents.push("owner-stopped"); + return state; + }); + const listDescendantProcessIdentities = timerControl.listDescendantProcessIdentities; + vi.spyOn(timerControl, "listDescendantProcessIdentities").mockImplementation( + (...args: unknown[]) => { + const [rootPid, deadline] = args as [number, number?]; + rootPid === owner.pid && takeoverEvents.push("owner-enumerated"); + return listDescendantProcessIdentities(rootPid, deadline); + }, + ); + const harness = createHarness(); fs.writeFileSync( transitionPath, JSON.stringify({ @@ -495,16 +555,185 @@ describe("shields command flow", () => { await new Promise((resolve) => setTimeout(resolve, 1400)); } finally { owner.kill("SIGKILL"); + try { + timerControl.readProcessStartIdentity(childPid) === childStartIdentity && + process.kill(childPid, "SIGKILL"); + } catch { + // The takeover already killed the exact child. + } } expect(fs.existsSync(sentinelPath)).toBe(false); expect(fs.existsSync(transitionPath)).toBe(false); + expect(takeoverEvents.indexOf("owner-stopped")).toBeGreaterThanOrEqual(0); + expect(takeoverEvents.indexOf("owner-enumerated")).toBeGreaterThan( + takeoverEvents.indexOf("owner-stopped"), + ); expect(harness.runSpy).toHaveBeenCalledWith( ["openshell", "policy", "set"], expect.objectContaining({ ignoreError: true }), ); }); + it("fails closed when the weakening subprocess set never reaches quiescence", { + timeout: 10_000, + }, () => { + const stateDir = path.join(tmpDir, ".nemoclaw", "state"); + fs.mkdirSync(stateDir, { recursive: true }); + const sandboxName = "non-quiescent"; + const processToken = "c".repeat(32); + const lockPath = path.join(stateDir, `shields-transition-lock-${sandboxName}.json`); + + const owner = spawn(process.execPath, ["-e", "setTimeout(() => {}, 5000)"], { + stdio: "ignore", + }); + expect(owner.pid).toBeTypeOf("number"); + const timerControl = requireDist("./timer-control.js"); + const ownerStartIdentity = timerControl.readProcessStartIdentity(owner.pid); + expect(ownerStartIdentity).toBeTypeOf("string"); + fs.writeFileSync( + lockPath, + JSON.stringify({ + version: 1, + sandboxName, + pid: owner.pid, + processStartIdentity: ownerStartIdentity, + command: "inference set", + acquiredAtMs: Date.now(), + takeoverToken: processToken, + }), + { mode: 0o600 }, + ); + + const syntheticPidBase = 2_000_000_000; + const readProcessStartIdentity = timerControl.readProcessStartIdentity; + vi.spyOn(timerControl, "readProcessStartIdentity").mockImplementation((...args: unknown[]) => { + const [pid, deadline] = args as [number, number?]; + return pid === owner.pid + ? ownerStartIdentity + : pid >= syntheticPidBase + ? `synthetic:${String(pid)}` + : readProcessStartIdentity(pid, deadline); + }); + const readProcessState = timerControl.readProcessState; + vi.spyOn(timerControl, "readProcessState").mockImplementation((...args: unknown[]) => { + const [pid, deadline] = args as [number, number?]; + return pid === owner.pid ? "T" : readProcessState(pid, deadline); + }); + const listDescendantProcessIdentities = timerControl.listDescendantProcessIdentities; + let ownerEnumerationPass = 0; + vi.spyOn(timerControl, "listDescendantProcessIdentities").mockImplementation( + (...args: unknown[]) => { + const [rootPid, deadline] = args as [number, number?]; + const ownerEnumeration = rootPid === owner.pid; + ownerEnumerationPass += ownerEnumeration ? 1 : 0; + const syntheticPid = syntheticPidBase + ownerEnumerationPass; + return ownerEnumeration + ? [{ pid: syntheticPid, startIdentity: `synthetic:${String(syntheticPid)}`, depth: 1 }] + : rootPid === process.pid + ? [] + : listDescendantProcessIdentities(rootPid, deadline); + }, + ); + vi.spyOn(Atomics, "wait").mockReturnValue("timed-out"); + const processKillSpy = vi.spyOn(process, "kill"); + createHarness(); + const shields = requireDist(shieldsModulePath) as { + prepareAutoRestoreTransitionTakeover: ( + sandboxName: string, + processToken: string, + snapshotPath: string, + ) => void; + }; + + try { + expect(() => + shields.prepareAutoRestoreTransitionTakeover( + sandboxName, + processToken, + path.join(stateDir, "unused-snapshot.yaml"), + ), + ).toThrow("Timed-out shields-down process tree could not be frozen safely"); + expect(processKillSpy).toHaveBeenCalledWith(owner.pid, "SIGSTOP"); + expect(processKillSpy).not.toHaveBeenCalledWith(owner.pid, "SIGKILL"); + } finally { + owner.kill("SIGCONT"); + owner.kill("SIGKILL"); + } + + expect(ownerEnumerationPass).toBe(8); + expect(fs.existsSync(lockPath)).toBe(true); + }); + + it("does not signal a replacement that reuses the owner PID during final verification", () => { + const stateDir = path.join(tmpDir, ".nemoclaw", "state"); + fs.mkdirSync(stateDir, { recursive: true }); + const sandboxName = "reused-owner"; + const processToken = "d".repeat(32); + const lockPath = path.join(stateDir, `shields-transition-lock-${sandboxName}.json`); + const owner = spawn(process.execPath, ["-e", "setTimeout(() => {}, 5000)"], { + stdio: "ignore", + }); + expect(owner.pid).toBeTypeOf("number"); + const timerControl = requireDist("./timer-control.js"); + const ownerStartIdentity = timerControl.readProcessStartIdentity(owner.pid); + expect(ownerStartIdentity).toBeTypeOf("string"); + fs.writeFileSync( + lockPath, + JSON.stringify({ + version: 1, + sandboxName, + pid: owner.pid, + processStartIdentity: ownerStartIdentity, + command: "config set write", + acquiredAtMs: Date.now(), + takeoverToken: processToken, + }), + { mode: 0o600 }, + ); + + const processKill = process.kill; + let ownerLivenessChecks = 0; + let replacementVisible = false; + const processKillSpy = vi.spyOn(process, "kill").mockImplementation((...args: unknown[]) => { + const [pid, signal] = args as [number, NodeJS.Signals | 0 | undefined]; + const ownerLivenessCheck = pid === owner.pid && signal === 0; + ownerLivenessChecks += ownerLivenessCheck ? 1 : 0; + replacementVisible ||= ownerLivenessCheck && ownerLivenessChecks === 2; + return processKill(pid, signal); + }); + const readProcessStartIdentity = timerControl.readProcessStartIdentity; + vi.spyOn(timerControl, "readProcessStartIdentity").mockImplementation((...args: unknown[]) => { + const [pid, deadline] = args as [number, number?]; + return pid === owner.pid && replacementVisible + ? "replacement-process-start" + : readProcessStartIdentity(pid, deadline); + }); + createHarness(); + const shields = requireDist(shieldsModulePath) as { + prepareAutoRestoreTransitionTakeover: ( + sandboxName: string, + processToken: string, + snapshotPath: string, + ) => void; + }; + + try { + shields.prepareAutoRestoreTransitionTakeover( + sandboxName, + processToken, + path.join(stateDir, "unused-snapshot.yaml"), + ); + expect(processKillSpy).not.toHaveBeenCalledWith(owner.pid, "SIGSTOP"); + expect(processKillSpy).not.toHaveBeenCalledWith(owner.pid, "SIGKILL"); + } finally { + owner.kill("SIGCONT"); + owner.kill("SIGKILL"); + } + + expect(ownerLivenessChecks).toBeGreaterThanOrEqual(2); + }); + it("preempts timer-token config and inference mutations at the restore deadline", async () => { const shields = requireDist(shieldsModulePath) as { prepareAutoRestoreTransitionTakeover: ( diff --git a/src/lib/shields/index.ts b/src/lib/shields/index.ts index 6546a4fd6f..8556619ad5 100644 --- a/src/lib/shields/index.ts +++ b/src/lib/shields/index.ts @@ -46,8 +46,11 @@ const { readTimerMarker, clearTimerMarker, isProcessAlive, + readProcessState, readProcessStartIdentity, listDescendantProcessIdentities, + processInspectionDeadlineAfter, + processInspectionDeadlineReached, verifyTimerMarkerIdentity, killTimer, } = require("./timer-control"); @@ -195,6 +198,30 @@ function clearShieldsDownTransition(sandboxName: string, processToken: string): } } +type ExactProcessStatus = "current" | "gone" | "unknown"; + +function processCanBeSignaled(pid: number): boolean { + if (!Number.isInteger(pid) || pid <= 0) return false; + try { + process.kill(pid, 0); + return true; + } catch (error) { + return (error as NodeJS.ErrnoException).code === "EPERM"; + } +} + +function readExactProcessStatus( + pid: number, + startIdentity: string, + deadline: number, +): ExactProcessStatus { + const alive = processCanBeSignaled(pid); + const observedStartIdentity = readProcessStartIdentity(pid, deadline); + if (observedStartIdentity === null) return alive ? "unknown" : "gone"; + if (observedStartIdentity !== startIdentity) return "gone"; + return alive ? "current" : "gone"; +} + function waitForShieldsDownForwardCommit( sandboxName: string, processToken: string, @@ -202,11 +229,15 @@ function waitForShieldsDownForwardCommit( let observed = readShieldsDownTransition(sandboxName, processToken); if (!observed) return null; - const ownerIsCurrent = () => - isProcessAlive(observed!.ownerPid) && - readProcessStartIdentity(observed!.ownerPid) === observed!.ownerStartIdentity; - const handoffDeadline = Date.now() + SHIELDS_TRANSITION_HANDOFF_GRACE_MS; - while (observed.phase === "preparing" && ownerIsCurrent() && Date.now() < handoffDeadline) { + const handoffDeadline = processInspectionDeadlineAfter(SHIELDS_TRANSITION_HANDOFF_GRACE_MS); + const ownerMayBeCurrent = () => + readExactProcessStatus(observed!.ownerPid, observed!.ownerStartIdentity, handoffDeadline) !== + "gone"; + while ( + observed.phase === "preparing" && + !processInspectionDeadlineReached(handoffDeadline) && + ownerMayBeCurrent() + ) { Atomics.wait(transitionPollBuffer, 0, 0, SHIELDS_TRANSITION_POLL_MS); const next = readShieldsDownTransition(sandboxName, processToken); if (!next) return null; @@ -221,7 +252,7 @@ function waitForShieldsDownForwardCommit( observed = next; } - if (observed.phase === "preparing" && ownerIsCurrent()) { + if (observed.phase === "preparing") { // The absolute shields-down deadline has expired while the forward owner // is still able to weaken policy/config. Preempt that exact process // instance, then restore from the captured snapshot. Waiting forever would @@ -233,18 +264,38 @@ function waitForShieldsDownForwardCommit( function excludeRecoveryProcessTree( descendants: ProcessIdentity[], - recoveryPid: number, + recovery: Pick, recoveryDescendants: ProcessIdentity[], ): ProcessIdentity[] { - const excludedPids = new Set([recoveryPid, ...recoveryDescendants.map(({ pid }) => pid)]); - return descendants.filter(({ pid }) => !excludedPids.has(pid)); + const identityKey = ({ pid, startIdentity }: Pick) => + `${String(pid)}\0${startIdentity}`; + const excludedIdentities = new Set([recovery, ...recoveryDescendants].map(identityKey)); + return descendants.filter((descendant) => !excludedIdentities.has(identityKey(descendant))); } function stopTimedOutShieldsDownTree(ownerPid: number, ownerStartIdentity: string): void { - const identityIsCurrent = (pid: number, startIdentity: string) => - isProcessAlive(pid) && readProcessStartIdentity(pid) === startIdentity; - const signalExact = (pid: number, startIdentity: string, signal: NodeJS.Signals): void => { - if (!identityIsCurrent(pid, startIdentity)) return; + let freezeDeadline = processInspectionDeadlineAfter(SHIELDS_TRANSITION_TERMINATE_GRACE_MS); + const waitForKnownExactProcess = ( + pid: number, + startIdentity: string, + deadline: number, + ): Exclude => { + while (true) { + const status = readExactProcessStatus(pid, startIdentity, deadline); + if (status !== "unknown") return status; + if (processInspectionDeadlineReached(deadline)) { + throw new Error("Timed-out shields-down process identity could not be verified safely"); + } + Atomics.wait(transitionPollBuffer, 0, 0, SHIELDS_TRANSITION_POLL_MS); + } + }; + const signalExact = ( + pid: number, + startIdentity: string, + signal: NodeJS.Signals, + deadline: number, + ): void => { + if (waitForKnownExactProcess(pid, startIdentity, deadline) === "gone") return; try { process.kill(pid, signal); } catch (error) { @@ -252,18 +303,39 @@ function stopTimedOutShieldsDownTree(ownerPid: number, ownerStartIdentity: strin if (errno.code !== "ESRCH") throw error; } }; - if (!identityIsCurrent(ownerPid, ownerStartIdentity)) return; - - const recoveryTree = listDescendantProcessIdentities(process.pid); + const waitForExactStop = (pid: number, startIdentity: string): "gone" | "stopped" => { + while (true) { + const state = readProcessState(pid, freezeDeadline); + const status = readExactProcessStatus(pid, startIdentity, freezeDeadline); + if (status === "gone" || state?.startsWith("Z")) return "gone"; + if (status === "current" && /^[Tt]/.test(state ?? "")) return "stopped"; + if (processInspectionDeadlineReached(freezeDeadline)) { + throw new Error("Timed-out shields-down process tree could not be frozen safely"); + } + Atomics.wait(transitionPollBuffer, 0, 0, SHIELDS_TRANSITION_POLL_MS); + } + }; + if (waitForKnownExactProcess(ownerPid, ownerStartIdentity, freezeDeadline) === "gone") return; + // Stop the exact owner before enumerating its descendants so it cannot launch + // another weakening subprocess while takeover is being established. + signalExact(ownerPid, ownerStartIdentity, "SIGSTOP", freezeDeadline); + if (waitForExactStop(ownerPid, ownerStartIdentity) === "gone") return; + freezeDeadline = processInspectionDeadlineAfter(SHIELDS_TRANSITION_TERMINATE_GRACE_MS); + const recoveryStartIdentity = readProcessStartIdentity(process.pid, freezeDeadline); + if (recoveryStartIdentity === null) { + throw new Error("Cannot identify the auto-restore recovery process safely"); + } + const recoveryTree = listDescendantProcessIdentities(process.pid, freezeDeadline); if (recoveryTree === null) { throw new Error("Cannot identify the auto-restore recovery process tree safely"); } - // Stop the exact owner before enumerating its descendants so it cannot launch - // another weakening subprocess while takeover is being established. - signalExact(ownerPid, ownerStartIdentity, "SIGSTOP"); const tracked = new Map(); + let observedQuiescentPass = false; for (let pass = 0; pass < 8; pass += 1) { - const descendants = listDescendantProcessIdentities(ownerPid); + if (waitForExactStop(ownerPid, ownerStartIdentity) === "gone") { + throw new Error("Timed-out shields-down process tree could not be frozen safely"); + } + const descendants = listDescendantProcessIdentities(ownerPid, freezeDeadline); if (descendants === null) { throw new Error("Cannot enumerate timed-out shields-down subprocesses safely"); } @@ -271,34 +343,54 @@ function stopTimedOutShieldsDownTree(ownerPid: number, ownerStartIdentity: strin const recoveryIsInsideOwnerTree = descendants.some( ({ pid }: { pid: number }) => pid === process.pid, ); - for (const descendant of excludeRecoveryProcessTree( + const passDescendants = excludeRecoveryProcessTree( descendants, - process.pid, + { pid: process.pid, startIdentity: recoveryStartIdentity }, recoveryIsInsideOwnerTree ? recoveryTree : [], - )) { - if (!tracked.has(descendant.pid)) added = true; + ); + for (const descendant of passDescendants) { + const previous = tracked.get(descendant.pid); + if (!previous || previous.startIdentity !== descendant.startIdentity) added = true; tracked.set(descendant.pid, { startIdentity: descendant.startIdentity, depth: descendant.depth, }); - signalExact(descendant.pid, descendant.startIdentity, "SIGSTOP"); + signalExact(descendant.pid, descendant.startIdentity, "SIGSTOP", freezeDeadline); + } + for (const descendant of passDescendants) { + waitForExactStop(descendant.pid, descendant.startIdentity); + } + if (!added) { + observedQuiescentPass = true; + break; } - if (!added) break; Atomics.wait(transitionPollBuffer, 0, 0, SHIELDS_TRANSITION_POLL_MS); } + if (!observedQuiescentPass) { + throw new Error("Timed-out shields-down process tree could not be frozen safely"); + } const deepestFirst = [...tracked.entries()].sort((a, b) => b[1].depth - a[1].depth); + const killDeadline = processInspectionDeadlineAfter(SHIELDS_TRANSITION_TERMINATE_GRACE_MS); for (const [pid, identity] of deepestFirst) { - signalExact(pid, identity.startIdentity, "SIGKILL"); + signalExact(pid, identity.startIdentity, "SIGKILL", killDeadline); } - signalExact(ownerPid, ownerStartIdentity, "SIGKILL"); + signalExact(ownerPid, ownerStartIdentity, "SIGKILL", killDeadline); - const killDeadline = Date.now() + SHIELDS_TRANSITION_TERMINATE_GRACE_MS; - while (Date.now() < killDeadline) { - const survivor = deepestFirst.some(([pid, identity]) => - identityIsCurrent(pid, identity.startIdentity), + const exactProcessIsGone = (pid: number, startIdentity: string): boolean => { + const state = readProcessState(pid, killDeadline); + return ( + state?.startsWith("Z") === true || + readExactProcessStatus(pid, startIdentity, killDeadline) === "gone" + ); + }; + while (!processInspectionDeadlineReached(killDeadline)) { + const survivor = deepestFirst.some( + ([pid, identity]) => !exactProcessIsGone(pid, identity.startIdentity), ); - if (!survivor && !identityIsCurrent(ownerPid, ownerStartIdentity)) return; + if (!survivor && exactProcessIsGone(ownerPid, ownerStartIdentity)) { + return; + } Atomics.wait(transitionPollBuffer, 0, 0, SHIELDS_TRANSITION_POLL_MS); } throw new Error("Timed-out shields-down process tree could not be stopped safely"); @@ -2158,15 +2250,11 @@ function prepareAutoRestoreTransitionTakeover( const owner = inspectShieldsTransitionLockOwner(sandboxName, processToken); if (!owner) return; - if ( - isProcessAlive(owner.pid) && - readProcessStartIdentity(owner.pid) === owner.processStartIdentity - ) { - // The same timer token is also propagated to config/inference/restart - // mutations made during the mutable window. At expiry those operations - // are weaker than restoring lockdown and may be preempted safely. - stopTimedOutShieldsDownTree(owner.pid, owner.processStartIdentity); - } + // The same timer token is also propagated to config/inference/restart + // mutations made during the mutable window. At expiry those operations + // are weaker than restoring lockdown and may be preempted safely. The stop + // helper pins the exact identity and fails closed if it cannot be read. + stopTimedOutShieldsDownTree(owner.pid, owner.processStartIdentity); const takeover = takeoverShieldsTransitionLock( sandboxName, owner.pid, diff --git a/src/lib/shields/timer-control.test.ts b/src/lib/shields/timer-control.test.ts new file mode 100644 index 0000000000..8645175177 --- /dev/null +++ b/src/lib/shields/timer-control.test.ts @@ -0,0 +1,25 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { performance } from "node:perf_hooks"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { processInspectionDeadlineAfter, processInspectionDeadlineReached } from "./timer-control"; + +describe("process inspection deadlines", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("remain bounded when the wall clock moves backward", () => { + const monotonicNow = vi.spyOn(performance, "now").mockReturnValue(1_000); + const wallClock = vi.spyOn(Date, "now").mockReturnValue(50_000); + const deadline = processInspectionDeadlineAfter(500); + + wallClock.mockReturnValue(-50_000); + monotonicNow.mockReturnValue(1_499); + expect(processInspectionDeadlineReached(deadline)).toBe(false); + + monotonicNow.mockReturnValue(1_500); + expect(processInspectionDeadlineReached(deadline)).toBe(true); + }); +}); diff --git a/src/lib/shields/timer-control.ts b/src/lib/shields/timer-control.ts index 56019263f0..727a89b734 100644 --- a/src/lib/shields/timer-control.ts +++ b/src/lib/shields/timer-control.ts @@ -4,10 +4,30 @@ import { execFileSync } from "node:child_process"; import fs from "node:fs"; import path from "node:path"; +import { performance } from "node:perf_hooks"; import { isObjectRecord } from "../core/json-types"; import { resolveNemoclawStateDir } from "../state/paths"; +const DEFAULT_PROCESS_INSPECTION_TIMEOUT_MS = 5_000; + +function processInspectionDeadline(deadline?: number): number { + return deadline ?? processInspectionDeadlineAfter(DEFAULT_PROCESS_INSPECTION_TIMEOUT_MS); +} + +function remainingProcessInspectionTimeout(deadline: number): number | null { + const remaining = deadline - performance.now(); + return remaining > 0 ? Math.max(1, Math.floor(remaining)) : null; +} + +function processInspectionDeadlineAfter(timeoutMs: number): number { + return performance.now() + timeoutMs; +} + +function processInspectionDeadlineReached(deadline: number): boolean { + return performance.now() >= deadline; +} + interface TimerMarker { pid: number; sandboxName: string; @@ -94,32 +114,41 @@ function clearTimerMarker(sandboxName: string): ClearTimerMarkerResult { } } -function isProcessAlive(pid: number): boolean { - if (!Number.isInteger(pid) || pid <= 0) return false; +function readProcessState(pid: number, deadline = processInspectionDeadline()): string | null { + if (!Number.isInteger(pid) || pid <= 0) return null; + if (remainingProcessInspectionTimeout(deadline) === null) return null; try { const raw = fs.readFileSync(`/proc/${String(pid)}/stat`, "utf-8"); const closingParen = raw.lastIndexOf(")"); - if ( - closingParen >= 0 && - raw + if (closingParen >= 0) { + const state = raw .slice(closingParen + 2) .trim() - .split(/\s+/, 1)[0] === "Z" - ) { - return false; + .split(/\s+/, 1)[0]; + if (state) return state; } } catch { - try { - const state = execFileSync("ps", ["-o", "stat=", "-p", String(pid)], { + // Fall through to the portable ps state. + } + try { + const timeout = remainingProcessInspectionTimeout(deadline); + if (timeout === null) return null; + return ( + execFileSync("ps", ["-o", "stat=", "-p", String(pid)], { stdio: ["ignore", "pipe", "ignore"], + timeout, }) .toString() - .trim(); - if (state.startsWith("Z")) return false; - } catch { - // Fall through to kill(0), which supplies the final liveness answer. - } + .trim() || null + ); + } catch { + return null; } +} + +function isProcessAlive(pid: number, deadline = processInspectionDeadline()): boolean { + if (!Number.isInteger(pid) || pid <= 0) return false; + if (readProcessState(pid, deadline)?.startsWith("Z")) return false; try { process.kill(pid, 0); return true; @@ -128,8 +157,12 @@ function isProcessAlive(pid: number): boolean { } } -function readProcessStartIdentity(pid: number): string | null { +function readProcessStartIdentity( + pid: number, + deadline = processInspectionDeadline(), +): string | null { if (!Number.isInteger(pid) || pid <= 0) return null; + if (remainingProcessInspectionTimeout(deadline) === null) return null; try { const raw = fs.readFileSync(`/proc/${String(pid)}/stat`, "utf-8"); const closingParen = raw.lastIndexOf(")"); @@ -146,8 +179,11 @@ function readProcessStartIdentity(pid: number): string | null { } try { + const timeout = remainingProcessInspectionTimeout(deadline); + if (timeout === null) return null; const started = execFileSync("ps", ["-o", "lstart=", "-p", String(pid)], { stdio: ["ignore", "pipe", "ignore"], + timeout, }) .toString() .trim(); @@ -163,12 +199,18 @@ interface ProcessIdentity { depth: number; } -function listDescendantProcessIdentities(rootPid: number): ProcessIdentity[] | null { +function listDescendantProcessIdentities( + rootPid: number, + deadline = processInspectionDeadline(), +): ProcessIdentity[] | null { if (!Number.isInteger(rootPid) || rootPid <= 0) return null; let rows: Array<{ pid: number; ppid: number }> = []; try { + const timeout = remainingProcessInspectionTimeout(deadline); + if (timeout === null) return null; rows = execFileSync("ps", ["-e", "-o", "pid=,ppid="], { stdio: ["ignore", "pipe", "ignore"], + timeout, }) .toString() .split("\n") @@ -199,10 +241,10 @@ function listDescendantProcessIdentities(rootPid: number): ProcessIdentity[] | n const identities: ProcessIdentity[] = []; for (const { pid, depth } of descendants) { - const startIdentity = readProcessStartIdentity(pid); + const startIdentity = readProcessStartIdentity(pid, deadline); if (startIdentity) { identities.push({ pid, startIdentity, depth }); - } else if (isProcessAlive(pid)) { + } else if (isProcessAlive(pid, deadline)) { // A live descendant that cannot be identity-pinned must not be signaled; // callers fail closed instead of risking PID-reuse collateral damage. return null; @@ -211,7 +253,12 @@ function listDescendantProcessIdentities(rootPid: number): ProcessIdentity[] | n return identities.sort((a, b) => b.depth - a.depth); } -function readProcessCommandLine(pid: number): string | null { +function readProcessCommandLine( + pid: number, + deadline = processInspectionDeadline(), +): string | null { + if (!Number.isInteger(pid) || pid <= 0) return null; + if (remainingProcessInspectionTimeout(deadline) === null) return null; const procCmdline = `/proc/${String(pid)}/cmdline`; try { if (fs.existsSync(procCmdline)) { @@ -223,8 +270,11 @@ function readProcessCommandLine(pid: number): string | null { } try { + const timeout = remainingProcessInspectionTimeout(deadline); + if (timeout === null) return null; const psCommand = execFileSync("ps", ["-o", "command=", "-p", String(pid)], { stdio: ["ignore", "pipe", "ignore"], + timeout, }) .toString() .trim(); @@ -322,8 +372,11 @@ export { isProcessAlive, killTimer, listDescendantProcessIdentities, + processInspectionDeadlineAfter, + processInspectionDeadlineReached, readAutoRestoreTakeoverToken, readProcessStartIdentity, + readProcessState, readTimerMarker, timerMarkerPath, verifyTimerMarkerIdentity, diff --git a/src/lib/tunnel/gateway-stop-script.test.ts b/src/lib/tunnel/gateway-stop-script.test.ts index c7b2a33113..569ea0da36 100644 --- a/src/lib/tunnel/gateway-stop-script.test.ts +++ b/src/lib/tunnel/gateway-stop-script.test.ts @@ -68,8 +68,8 @@ describe("GATEWAY_STOP_SCRIPT (executed)", () => { const scopedScript = ` allowed_test_pids="${allowedPids.join(" ")}" ps() { - if [ "$*" = "-eo user=,pid=,args=" ]; then - command ps -eo user=,pid=,args= | awk -v allowed="$allowed_test_pids" ' + if [ "$*" = "-eo uid=,pid=,args=" ]; then + command ps -eo uid=,pid=,args= | awk -v allowed="$allowed_test_pids" ' BEGIN { split(allowed, pids, " ") for (i in pids) if (pids[i] != "") keep[pids[i]] = 1 @@ -107,6 +107,8 @@ ${script}`; mode = 0o600, pidContent = `${pid} ${processStartTime(pid)}\n`, ): { script: string; pidFile: string; markerFile: string } { + const currentUid = process.getuid?.(); + assert(currentUid !== undefined, "gateway stop identity tests require a numeric UID"); const dir = mkdtempSync(join(tmpdir(), "nemoclaw-gateway-stop-identity-")); identityDirs.push(dir); const pidFile = join(dir, "nemoclaw-gateway.pid"); @@ -117,11 +119,7 @@ ${script}`; chmodSync(markerFile, mode); const script = GATEWAY_STOP_SCRIPT.replaceAll("/tmp/nemoclaw-gateway.pid", pidFile) .replaceAll("/tmp/nemoclaw-gateway-local", markerFile) - .replace( - 'allowed_bare_users="gateway,sandbox"', - `allowed_bare_users="gateway,sandbox,${process.env.USER ?? ""}"`, - ) - .replace("root|gateway|sandbox) ;;", `root|gateway|sandbox|${process.env.USER ?? ""}) ;;`); + .replace('allowed_bare_uids=","', `allowed_bare_uids=",${currentUid},"`); return { script, pidFile, markerFile }; } @@ -218,12 +216,12 @@ ${script}`; const decoy = spawnWithArgv0("openclaw"); const { script, pidFile } = identityFixture(intended); const replacement = `${decoy} ${processStartTime(decoy)}`; - const replaceAfterOpen = `marker_owner="$(trusted_identity_fd "/proc/$$/fd/4" || true)" + const replaceAfterOpen = `marker_owner_uid="$(trusted_identity_fd "/proc/$$/fd/4" || true)" mv "${pidFile}" "${pidFile}.opened" printf '%s\\n' '${replacement}' >"${pidFile}" chmod 600 "${pidFile}"`; const racedScript = script.replace( - 'marker_owner="$(trusted_identity_fd "/proc/$$/fd/4" || true)"', + 'marker_owner_uid="$(trusted_identity_fd "/proc/$$/fd/4" || true)"', replaceAfterOpen, ); @@ -253,12 +251,13 @@ ${script}`; ); it.runIf(process.platform === "linux")( - "spares bare openclaw when gateway identity owner mismatches the process user", - () => { + "spares bare openclaw when gateway identity owner UID mismatches the process UID", + async () => { const decoy = spawnWithArgv0("openclaw"); + await waitForArgv0(decoy, "openclaw"); const script = stopScriptWithGatewayIdentity(decoy).replace( - '-v identity_owner="$pidfile_owner"', - '-v identity_owner="sandbox"', + '-v identity_owner_uid="$pidfile_owner_uid"', + '-v identity_owner_uid="99999999"', ); expect(runStopScript(script)).toBe(1); expect(isAlive(decoy)).toBe(true); diff --git a/src/lib/tunnel/gateway-stop-script.ts b/src/lib/tunnel/gateway-stop-script.ts index 57cd5bf51d..66bbdd21f1 100644 --- a/src/lib/tunnel/gateway-stop-script.ts +++ b/src/lib/tunnel/gateway-stop-script.ts @@ -35,11 +35,23 @@ parent="$PPID" gateway_pid_file="/tmp/nemoclaw-gateway.pid" gateway_marker_file="/tmp/nemoclaw-gateway-local" +# Resolve the supported gateway accounts to stable numeric IDs. GNU ps limits +# the displayed width of user names, so a long account cannot be compared +# reliably with stat's full owner name. +allowed_bare_uids="," +for allowed_bare_user in gateway sandbox; do + allowed_bare_uid="$(id -u "$allowed_bare_user" 2>/dev/null || true)" + case "$allowed_bare_uid" in + ''|0|*[!0-9]*) ;; + *) allowed_bare_uids="$allowed_bare_uids$allowed_bare_uid," ;; + esac +done + # Open both identity files once, then validate and read those exact file # descriptions through /proc. This prevents a same-owner process from swapping # a pathname in world-writable /tmp between validation and the PID read. -pidfile_owner="" -marker_owner="" +pidfile_owner_uid="" +marker_owner_uid="" if [ -f "$gateway_pid_file" ] && [ -f "$gateway_marker_file" ] && \ [ ! -L "$gateway_pid_file" ] && [ ! -L "$gateway_marker_file" ] && \ exec 3<"$gateway_pid_file" 4<"$gateway_marker_file"; then @@ -47,24 +59,24 @@ if [ -f "$gateway_pid_file" ] && [ -f "$gateway_marker_file" ] && \ fd_path="$1" [ -f "$fd_path" ] || return 1 mode="$(stat -Lc '%a' "$fd_path" 2>/dev/null)" || return 1 - owner="$(stat -Lc '%U' "$fd_path" 2>/dev/null)" || return 1 + owner_uid="$(stat -Lc '%u' "$fd_path" 2>/dev/null)" || return 1 case "$mode" in *00) ;; *) return 1 ;; esac - case "$owner" in - root|gateway|sandbox) ;; + case ",0$allowed_bare_uids" in + *,"$owner_uid",*) ;; *) return 1 ;; esac - printf '%s\n' "$owner" + printf '%s\n' "$owner_uid" } - pidfile_owner="$(trusted_identity_fd "/proc/$$/fd/3" || true)" - marker_owner="$(trusted_identity_fd "/proc/$$/fd/4" || true)" + pidfile_owner_uid="$(trusted_identity_fd "/proc/$$/fd/3" || true)" + marker_owner_uid="$(trusted_identity_fd "/proc/$$/fd/4" || true)" fi pidfile_pid="" identity_files_trusted=0 -if [ -n "$pidfile_owner" ] && [ "$pidfile_owner" = "$marker_owner" ]; then +if [ -n "$pidfile_owner_uid" ] && [ "$pidfile_owner_uid" = "$marker_owner_uid" ]; then IFS= read -r raw_pidfile_line <&3 || true raw_pidfile_pid="$(printf '%s\n' "$raw_pidfile_line" | awk '{ print $1 }')" raw_pidfile_starttime="$(printf '%s\n' "$raw_pidfile_line" | awk '{ print $2 }')" @@ -81,29 +93,28 @@ if [ -n "$pidfile_owner" ] && [ "$pidfile_owner" = "$marker_owner" ]; then esac fi -# A root-owned identity file can authorize either supported gateway user. A +# A root-owned identity file can authorize either supported gateway UID. A # gateway- or sandbox-owned identity can authorize only a process of that same -# user, enforced in the matcher below. -allowed_bare_users="gateway,sandbox" +# UID, enforced in the matcher below. find_gateway_pids() { - ps -eo user=,pid=,args= 2>/dev/null | awk \ + ps -eo uid=,pid=,args= 2>/dev/null | awk \ -v self="$self" \ -v parent="$parent" \ -v pidfile_pid="$pidfile_pid" \ -v identity_files_trusted="$identity_files_trusted" \ - -v identity_owner="$pidfile_owner" \ - -v allowed_bare_users="$allowed_bare_users" ' - function allowed_bare_user(user) { - return index("," allowed_bare_users ",", "," user ",") > 0 + -v identity_owner_uid="$pidfile_owner_uid" \ + -v allowed_bare_uids="$allowed_bare_uids" ' + function allowed_bare_uid(uid) { + return index(allowed_bare_uids, "," uid ",") > 0 } $2 ~ /^[0-9]+$/ && $2 != self && $2 != parent { - user = $1 + uid = $1 pid = $2 cmd = $0 sub(/^[[:space:]]*[^[:space:]]+[[:space:]]+[0-9]+[[:space:]]+/, "", cmd) if (cmd ~ /(^|[[:space:]\/])openclaw-gateway([[:space:]]|$)/ || cmd ~ /(^|[[:space:]\/])openclaw[[:space:]]+gateway([[:space:]]|$)/) { seen[pid] = 1 - } else if (identity_files_trusted == "1" && pid == pidfile_pid && allowed_bare_user(user) && (identity_owner == "root" || identity_owner == user) && cmd ~ /(^|[[:space:]\/])openclaw[[:space:]]*$/) { + } else if (identity_files_trusted == "1" && pid == pidfile_pid && allowed_bare_uid(uid) && (identity_owner_uid == "0" || identity_owner_uid == uid) && cmd ~ /(^|[[:space:]\/])openclaw[[:space:]]*$/) { seen[pid] = 1 } } diff --git a/src/lib/tunnel/sandbox-gateway-stop.test.ts b/src/lib/tunnel/sandbox-gateway-stop.test.ts index 8e1dda492d..4178109e63 100644 --- a/src/lib/tunnel/sandbox-gateway-stop.test.ts +++ b/src/lib/tunnel/sandbox-gateway-stop.test.ts @@ -91,7 +91,9 @@ describe("stopSandboxChannels", () => { expect.arrayContaining(["kubectl", "exec", "-n", "openshell", "-c", "agent"]), ); const script = String(args.at(-1)); - expect(script).toContain("ps -eo user=,pid=,args="); + expect(script).toContain("ps -eo uid=,pid=,args="); + expect(script).toContain("stat -Lc '%u'"); + expect(script).not.toContain("ps -eo user=,pid=,args="); expect(script).toContain("openclaw-gateway"); expect(script).toContain("kill -TERM $pids"); expect(script).toContain("kill -KILL $remaining"); diff --git a/test/helpers/hermes-restart-config-seal-fixture.ts b/test/helpers/hermes-restart-config-seal-fixture.ts index 763208ad15..1d5fbf6550 100644 --- a/test/helpers/hermes-restart-config-seal-fixture.ts +++ b/test/helpers/hermes-restart-config-seal-fixture.ts @@ -95,6 +95,19 @@ export function createRestartFixture(): RestartFixture { }; } +export function allowRestartFixturePeerTraversal(fixture: RestartFixture): () => void { + const testTempRoot = path.dirname(fixture.root); + const testTempRootMode = mode(testTempRoot); + fs.chmodSync(testTempRoot, testTempRootMode | 0o001); + try { + fs.chmodSync(fixture.root, mode(fixture.root) | 0o001); + } catch (error) { + fs.chmodSync(testTempRoot, testTempRootMode); + throw error; + } + return () => fs.chmodSync(testTempRoot, testTempRootMode); +} + export function runWriteConfig(fixture: RestartFixture, expectedDigest: string, content: string) { return spawnSync( "python3", diff --git a/test/hermes-restart-config-seal-recovery.test.ts b/test/hermes-restart-config-seal-recovery.test.ts index 5615f5f772..60b441c885 100644 --- a/test/hermes-restart-config-seal-recovery.test.ts +++ b/test/hermes-restart-config-seal-recovery.test.ts @@ -5,9 +5,10 @@ import { spawnSync } from "node:child_process"; import { randomBytes } from "node:crypto"; import fs from "node:fs"; import path from "node:path"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { + allowRestartFixturePeerTraversal, createRestartFixture, mode, overwriteThroughOldFd, @@ -20,14 +21,44 @@ import { } from "./helpers/hermes-restart-config-seal-fixture"; describe.skipIf(process.platform === "win32")("Hermes mutable restart input seal", () => { + it("restores parent traversal permissions when peer setup fails", () => { + const fixture = createRestartFixture(); + const isolatedParent = fs.mkdtempSync(path.join(path.dirname(fixture.root), "peer-setup-")); + const isolatedRoot = path.join(isolatedParent, "fixture"); + fs.mkdirSync(isolatedRoot, { mode: 0o700 }); + fs.chmodSync(isolatedParent, 0o700); + const isolatedFixture = { ...fixture, root: isolatedRoot }; + const realChmodSync = fs.chmodSync.bind(fs); + const chmod = vi + .spyOn(fs, "chmodSync") + .mockImplementationOnce(realChmodSync) + .mockImplementationOnce(() => { + throw new Error("fixture chmod failed"); + }) + .mockImplementation(realChmodSync); + + try { + expect(() => allowRestartFixturePeerTraversal(isolatedFixture)).toThrow( + "fixture chmod failed", + ); + expect(mode(isolatedParent)).toBe(0o700); + } finally { + chmod.mockRestore(); + fs.rmSync(isolatedParent, { recursive: true, force: true }); + fs.rmSync(fixture.root, { recursive: true, force: true }); + } + }); + it.runIf( process.platform === "linux" && process.getuid?.() === 0 && spawnSync("setpriv", ["--version"], { encoding: "utf-8" }).status === 0, )("keeps the locked Hermes entry sticky-protected while allowing ordinary home writes", () => { const fixture = createRestartFixture(); + let restoreTempRootMode: (() => void) | undefined; try { + restoreTempRootMode = allowRestartFixturePeerTraversal(fixture); const locked = runShieldsTransition(fixture, "locked"); expect(locked.status, locked.stderr).toBe(0); const parent = fs.statSync(fixture.sandboxDir); @@ -55,7 +86,16 @@ describe.skipIf(process.platform === "win32")("Hermes mutable restart input seal expect(fs.existsSync(fixture.hermesDir)).toBe(true); expect(fs.existsSync(path.join(fixture.sandboxDir, ".hermes-moved"))).toBe(false); } finally { - fs.rmSync(fixture.root, { recursive: true, force: true }); + try { + const mutable = runShieldsTransition(fixture, "mutable"); + expect(mutable.status, mutable.stderr).toBe(0); + } finally { + try { + fs.rmSync(fixture.root, { recursive: true, force: true }); + } finally { + restoreTempRootMode?.(); + } + } } }); @@ -316,8 +356,10 @@ describe.skipIf(process.platform === "win32")("Hermes mutable restart input seal spawnSync("setpriv", ["--version"], { encoding: "utf-8" }).status === 0, )("lets a sandbox-group peer create state but not unlink sealed config names", () => { const fixture = createRestartFixture(); + let restoreTempRootMode: (() => void) | undefined; try { + restoreTempRootMode = allowRestartFixturePeerTraversal(fixture); const sealed = runGuard("seal-restart", fixture); expect(sealed.status, sealed.stderr).toBe(0); @@ -344,7 +386,11 @@ describe.skipIf(process.platform === "win32")("Hermes mutable restart input seal const unsealed = runGuard("unseal-restart", fixture); expect(unsealed.status, unsealed.stderr).toBe(0); } finally { - fs.rmSync(fixture.root, { recursive: true, force: true }); + try { + fs.rmSync(fixture.root, { recursive: true, force: true }); + } finally { + restoreTempRootMode?.(); + } } }); }); diff --git a/test/platform-vitest-main-workflow.test.ts b/test/platform-vitest-main-workflow.test.ts index d3c3118ab5..5854a59d83 100644 --- a/test/platform-vitest-main-workflow.test.ts +++ b/test/platform-vitest-main-workflow.test.ts @@ -48,7 +48,7 @@ describe("platform Vitest main workflow", () => { cache: "pip", "cache-dependency-path": MACOS_REQUIREMENTS_PATH, }); - for (const dependency of ["bash", "coreutils", "gawk", "ripgrep"]) { + for (const dependency of ["bash", "coreutils", "fd", "gawk", "ripgrep"]) { expect(run).toMatch(new RegExp(`brew install[^\\n]*\\b${dependency}\\b`, "u")); } expect(run).toContain("$(brew --prefix bash)/bin");