mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-13 04:16:19 +00:00
fix(lock): require owner identity proof before stale removal
Fixes #86814. Reclaims stale plugin lock files only when the previous owner is provably gone or the recorded process start time proves PID reuse. Timestamp age alone now stays fail-closed for PID-owned locks, preserving mutual exclusion for long-running writers while still allowing pidless expired locks to expire. Verification: - pnpm test src/infra/stale-lock-file.test.ts src/plugin-sdk/file-lock.test.ts - pnpm tool-display:check - git diff --check - autoreview --mode branch --base origin/main Known CI note: check-guards failed in deps:shrinkwrap:check because npm resolved newer AWS transitive versions than pnpm-lock.yaml contains; no package or lock files are changed in this PR. Co-authored-by: Alix-007 <267018309+Alix-007@users.noreply.github.com>
This commit is contained in:
@@ -2,19 +2,52 @@ import { describe, expect, it } from "vitest";
|
||||
import { shouldRemoveDeadOwnerOrExpiredLock } from "./stale-lock-file.js";
|
||||
|
||||
describe("stale lock file ownership", () => {
|
||||
it("treats permission-denied process probes as not definitely dead", () => {
|
||||
it("keeps expired locks when a pid owner is not definitely dead", () => {
|
||||
expect(
|
||||
shouldRemoveDeadOwnerOrExpiredLock({
|
||||
payload: {
|
||||
pid: 123,
|
||||
createdAt: new Date(Date.now() - 60_000).toISOString(),
|
||||
createdAt: "2026-05-23T00:00:00.000Z",
|
||||
},
|
||||
staleMs: 10,
|
||||
nowMs: Date.parse("2026-05-23T00:00:11.000Z"),
|
||||
isPidDefinitelyDead: () => false,
|
||||
}),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("removes locks when the owner pid starttime changed", () => {
|
||||
expect(
|
||||
shouldRemoveDeadOwnerOrExpiredLock({
|
||||
payload: {
|
||||
pid: 123,
|
||||
createdAt: "2026-05-23T00:00:00.000Z",
|
||||
starttime: 111,
|
||||
},
|
||||
staleMs: 60_000,
|
||||
nowMs: Date.parse("2026-05-23T00:00:10.000Z"),
|
||||
isPidDefinitelyDead: () => false,
|
||||
getProcessStartTime: () => 222,
|
||||
}),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("does not remove locks when the owner pid starttime still matches", () => {
|
||||
expect(
|
||||
shouldRemoveDeadOwnerOrExpiredLock({
|
||||
payload: {
|
||||
pid: 123,
|
||||
createdAt: "2026-05-23T00:00:00.000Z",
|
||||
starttime: 111,
|
||||
},
|
||||
staleMs: 10,
|
||||
nowMs: Date.parse("2026-05-23T00:00:11.000Z"),
|
||||
isPidDefinitelyDead: () => false,
|
||||
getProcessStartTime: () => 111,
|
||||
}),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("only removes pid-owned locks when the owner is definitely dead", () => {
|
||||
expect(
|
||||
shouldRemoveDeadOwnerOrExpiredLock({
|
||||
@@ -27,4 +60,16 @@ describe("stale lock file ownership", () => {
|
||||
}),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("removes expired pidless locks", () => {
|
||||
expect(
|
||||
shouldRemoveDeadOwnerOrExpiredLock({
|
||||
payload: {
|
||||
createdAt: "2026-05-23T00:00:00.000Z",
|
||||
},
|
||||
staleMs: 10,
|
||||
nowMs: Date.parse("2026-05-23T00:00:11.000Z"),
|
||||
}),
|
||||
).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,8 +1,12 @@
|
||||
import { isPidDefinitelyDead as defaultIsPidDefinitelyDead } from "../shared/pid-alive.js";
|
||||
import {
|
||||
getProcessStartTime as defaultGetProcessStartTime,
|
||||
isPidDefinitelyDead as defaultIsPidDefinitelyDead,
|
||||
} from "../shared/pid-alive.js";
|
||||
|
||||
export type LockFileOwnerPayload = {
|
||||
pid?: number;
|
||||
createdAt?: string;
|
||||
starttime?: number;
|
||||
};
|
||||
|
||||
export function readLockFileOwnerPayload(
|
||||
@@ -14,6 +18,7 @@ export function readLockFileOwnerPayload(
|
||||
return {
|
||||
pid: typeof payload.pid === "number" ? payload.pid : undefined,
|
||||
createdAt: typeof payload.createdAt === "string" ? payload.createdAt : undefined,
|
||||
starttime: typeof payload.starttime === "number" ? payload.starttime : undefined,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -22,14 +27,27 @@ export function shouldRemoveDeadOwnerOrExpiredLock(params: {
|
||||
staleMs: number;
|
||||
nowMs?: number;
|
||||
isPidDefinitelyDead?: (pid: number) => boolean;
|
||||
getProcessStartTime?: (pid: number) => number | null;
|
||||
}): boolean {
|
||||
const payload = readLockFileOwnerPayload(params.payload);
|
||||
if (payload?.pid) {
|
||||
// Timestamp age alone cannot prove the owner stopped writing. Only a
|
||||
// mismatched process start time proves PID reuse while the PID is alive.
|
||||
if (payload.starttime !== undefined) {
|
||||
const currentStarttime = (params.getProcessStartTime ?? defaultGetProcessStartTime)(
|
||||
payload.pid,
|
||||
);
|
||||
if (currentStarttime !== null && currentStarttime !== payload.starttime) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return (params.isPidDefinitelyDead ?? defaultIsPidDefinitelyDead)(payload.pid);
|
||||
}
|
||||
if (payload?.createdAt) {
|
||||
const createdAt = Date.parse(payload.createdAt);
|
||||
return !Number.isFinite(createdAt) || (params.nowMs ?? Date.now()) - createdAt > params.staleMs;
|
||||
if (!Number.isFinite(createdAt) || (params.nowMs ?? Date.now()) - createdAt > params.staleMs) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
@@ -117,7 +117,7 @@ describe("acquireFileLock", () => {
|
||||
await expect(fs.readFile(lockPath, "utf8")).resolves.toBe("{");
|
||||
});
|
||||
|
||||
it("keeps a reported stale lock when its owner pid is alive", async () => {
|
||||
it("keeps an expired lock when its live owner has no starttime proof", async () => {
|
||||
const filePath = path.join(tempDir, "live-owner");
|
||||
const lockPath = `${filePath}.lock`;
|
||||
const options = {
|
||||
|
||||
@@ -5,6 +5,7 @@ import {
|
||||
resetFileLockManagerForTest,
|
||||
} from "@openclaw/fs-safe/file-lock";
|
||||
import { shouldRemoveDeadOwnerOrExpiredLock } from "../infra/stale-lock-file.js";
|
||||
import { getProcessStartTime } from "../shared/pid-alive.js";
|
||||
|
||||
export type FileLockOptions = {
|
||||
retries: {
|
||||
@@ -86,7 +87,17 @@ export async function acquireFileLock(
|
||||
retry: options.retries,
|
||||
staleRecovery: "remove-if-unchanged",
|
||||
allowReentrant: true,
|
||||
payload: () => ({ pid: process.pid, createdAt: new Date().toISOString() }),
|
||||
payload: () => {
|
||||
const payload: Record<string, unknown> = {
|
||||
pid: process.pid,
|
||||
createdAt: new Date().toISOString(),
|
||||
};
|
||||
const starttime = getProcessStartTime(process.pid);
|
||||
if (starttime !== null) {
|
||||
payload.starttime = starttime;
|
||||
}
|
||||
return payload;
|
||||
},
|
||||
shouldReclaim: shouldReclaimPluginLock,
|
||||
shouldRemoveStaleLock: (snapshot) =>
|
||||
shouldRemoveDeadOwnerOrExpiredLock({
|
||||
|
||||
Reference in New Issue
Block a user