mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-21 10:16:44 +00:00
fix: release session lock before runtime teardown (#87747)
Summary: - The PR reorders embedded attempt cleanup to release the session write lock before session/MCP/LSP teardown, treats sessions_yield cleanup as abort-like for flush timing, and adds focused regression tests. - PR surface: Source +14, Tests +71. Total +85 across 3 files. - Reproducibility: yes. Source inspection shows current main releases the cleanup lock only after runtime tear ... R body’s terminal proof exercises the same ordering with production cleanup and filesystem lock primitives. Automerge notes: - PR branch already contained follow-up commit before automerge: Merge branch 'main' into fix/session-lock-release-before-teardown Validation: - ClawSweeper review passed for head178192fa0e. - Required merge gates passed before the squash merge. Prepared head SHA:178192fa0eReview: https://github.com/openclaw/openclaw/pull/87747#issuecomment-4566994280 Co-authored-by: fuller-stack-dev <263060202+fuller-stack-dev@users.noreply.github.com> Co-authored-by: Jason (Json) <263060202+fuller-stack-dev@users.noreply.github.com> Co-authored-by: clawsweeper[bot] <274271284+clawsweeper[bot]@users.noreply.github.com> Approved-by: takhoffman Co-authored-by: takhoffman <781889+takhoffman@users.noreply.github.com>
This commit is contained in:
co-authored by
clawsweeper[bot] <274271284+clawsweeper[bot]@users.noreply.github.com>
takhoffman
parent
59997d8689
commit
0dbdaf98ea
@@ -21,7 +21,7 @@ describe("cleanupEmbeddedAttemptResources", () => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("waits for aborted prompt settlement before flushing, disposing, and releasing the lock", async () => {
|
||||
it("waits for aborted prompt settlement before flushing and releasing the lock", async () => {
|
||||
const order: string[] = [];
|
||||
const settle = createDeferred<void>();
|
||||
|
||||
@@ -57,7 +57,7 @@ describe("cleanupEmbeddedAttemptResources", () => {
|
||||
settle.resolve();
|
||||
await cleanupPromise;
|
||||
|
||||
expect(order).toEqual(["guard", "flush", "dispose", "release"]);
|
||||
expect(order).toEqual(["guard", "flush", "release", "dispose"]);
|
||||
});
|
||||
|
||||
it("releases the lock after the aborted settle timeout", async () => {
|
||||
@@ -93,7 +93,44 @@ describe("cleanupEmbeddedAttemptResources", () => {
|
||||
await vi.advanceTimersByTimeAsync(1);
|
||||
await cleanupPromise;
|
||||
|
||||
expect(order).toEqual(["flush", "dispose", "release"]);
|
||||
expect(order).toEqual(["flush", "release", "dispose"]);
|
||||
});
|
||||
|
||||
it("releases the lock before runtime teardown can hang", async () => {
|
||||
const order: string[] = [];
|
||||
let markRuntimeDisposeStarted!: () => void;
|
||||
const runtimeDisposeStarted = new Promise<void>((resolve) => {
|
||||
markRuntimeDisposeStarted = resolve;
|
||||
});
|
||||
|
||||
void cleanupEmbeddedAttemptResources({
|
||||
flushPendingToolResultsAfterIdle: vi.fn(async () => {
|
||||
order.push("flush");
|
||||
}),
|
||||
session: {
|
||||
agent: {},
|
||||
dispose: () => {
|
||||
order.push("dispose");
|
||||
},
|
||||
},
|
||||
sessionManager: {},
|
||||
sessionLock: {
|
||||
release: async () => {
|
||||
order.push("release");
|
||||
},
|
||||
},
|
||||
bundleMcpRuntime: {
|
||||
dispose: async () => {
|
||||
order.push("runtime-dispose-start");
|
||||
markRuntimeDisposeStarted();
|
||||
await new Promise(() => {});
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
await runtimeDisposeStarted;
|
||||
|
||||
expect(order).toEqual(["flush", "release", "dispose", "runtime-dispose-start"]);
|
||||
});
|
||||
|
||||
it("does not wait for the settle promise on non-aborted cleanup", async () => {
|
||||
@@ -116,10 +153,43 @@ describe("cleanupEmbeddedAttemptResources", () => {
|
||||
expect(release).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("still disposes resources when lock release fails", async () => {
|
||||
const releaseError = new Error("release failed");
|
||||
const dispose = vi.fn();
|
||||
const runtimeDispose = vi.fn(async () => {});
|
||||
|
||||
await expect(
|
||||
cleanupEmbeddedAttemptResources({
|
||||
flushPendingToolResultsAfterIdle: vi.fn(async () => {}),
|
||||
session: {
|
||||
agent: {},
|
||||
dispose,
|
||||
},
|
||||
sessionManager: {},
|
||||
sessionLock: {
|
||||
release: async () => {
|
||||
throw releaseError;
|
||||
},
|
||||
},
|
||||
bundleMcpRuntime: {
|
||||
dispose: runtimeDispose,
|
||||
},
|
||||
}),
|
||||
).rejects.toBe(releaseError);
|
||||
|
||||
expect(dispose).toHaveBeenCalledTimes(1);
|
||||
expect(runtimeDispose).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("can skip stale session-manager flushing after session takeover", async () => {
|
||||
const flushPendingToolResultsAfterIdle = vi.fn(async () => {});
|
||||
const dispose = vi.fn();
|
||||
const release = vi.fn(async () => {});
|
||||
const order: string[] = [];
|
||||
const dispose = vi.fn(() => {
|
||||
order.push("dispose");
|
||||
});
|
||||
const release = vi.fn(async () => {
|
||||
order.push("release");
|
||||
});
|
||||
|
||||
await cleanupEmbeddedAttemptResources({
|
||||
flushPendingToolResultsAfterIdle,
|
||||
@@ -135,5 +205,6 @@ describe("cleanupEmbeddedAttemptResources", () => {
|
||||
expect(flushPendingToolResultsAfterIdle).not.toHaveBeenCalled();
|
||||
expect(dispose).toHaveBeenCalledTimes(1);
|
||||
expect(release).toHaveBeenCalledTimes(1);
|
||||
expect(order).toEqual(["release", "dispose"]);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -70,6 +70,7 @@ export async function cleanupEmbeddedAttemptResources(params: {
|
||||
runId?: string;
|
||||
sessionId?: string;
|
||||
}): Promise<void> {
|
||||
let sessionLockReleaseError: unknown;
|
||||
try {
|
||||
try {
|
||||
params.removeToolResultContextGuard?.();
|
||||
@@ -97,22 +98,31 @@ export async function cleanupEmbeddedAttemptResources(params: {
|
||||
/* best-effort */
|
||||
}
|
||||
}
|
||||
try {
|
||||
params.session?.dispose();
|
||||
} catch {
|
||||
/* best-effort */
|
||||
}
|
||||
try {
|
||||
await params.bundleMcpRuntime?.dispose();
|
||||
} catch {
|
||||
/* best-effort */
|
||||
}
|
||||
try {
|
||||
await params.bundleLspRuntime?.dispose();
|
||||
} catch {
|
||||
/* best-effort */
|
||||
}
|
||||
} finally {
|
||||
await params.sessionLock.release();
|
||||
try {
|
||||
await params.sessionLock.release();
|
||||
} catch (err) {
|
||||
sessionLockReleaseError = err;
|
||||
}
|
||||
}
|
||||
|
||||
try {
|
||||
params.session?.dispose();
|
||||
} catch {
|
||||
/* best-effort */
|
||||
}
|
||||
try {
|
||||
await params.bundleMcpRuntime?.dispose();
|
||||
} catch {
|
||||
/* best-effort */
|
||||
}
|
||||
try {
|
||||
await params.bundleLspRuntime?.dispose();
|
||||
} catch {
|
||||
/* best-effort */
|
||||
}
|
||||
|
||||
if (sessionLockReleaseError) {
|
||||
throw sessionLockReleaseError;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1797,6 +1797,7 @@ export async function runEmbeddedAttempt(
|
||||
let trajectoryRecorder: ReturnType<typeof createTrajectoryRuntimeRecorder> | null = null;
|
||||
let trajectoryEndRecorded = false;
|
||||
let buildAbortSettlePromise: () => Promise<void> | null = () => null;
|
||||
let cleanupYieldAborted = false;
|
||||
try {
|
||||
await repairSessionFileIfNeeded({
|
||||
sessionFile: params.sessionFile,
|
||||
@@ -4011,6 +4012,7 @@ export async function runEmbeddedAttempt(
|
||||
isRunnerAbortError(err) &&
|
||||
err instanceof Error &&
|
||||
err.cause === "sessions_yield";
|
||||
cleanupYieldAborted = yieldAborted;
|
||||
if (yieldAborted) {
|
||||
aborted = false;
|
||||
await waitForSessionsYieldAbortSettle({
|
||||
@@ -4762,6 +4764,7 @@ export async function runEmbeddedAttempt(
|
||||
timedOut ||
|
||||
idleTimedOut ||
|
||||
timedOutDuringCompaction;
|
||||
const cleanupAbortLike = cleanupAborted || cleanupYieldAborted;
|
||||
const cleanupSessionLock = await sessionLockController.acquireForCleanup({ session });
|
||||
await cleanupEmbeddedAttemptResources({
|
||||
removeToolResultContextGuard,
|
||||
@@ -4771,9 +4774,10 @@ export async function runEmbeddedAttempt(
|
||||
bundleMcpRuntime,
|
||||
bundleLspRuntime,
|
||||
sessionLock: cleanupSessionLock,
|
||||
// PERF: If the run was aborted (user stop, timeout, etc.), skip the idle wait
|
||||
// and flush pending results synchronously so we can release the session lock ASAP.
|
||||
aborted: cleanupAborted,
|
||||
// PERF: If the run was aborted (user stop, timeout, sessions_yield, etc.),
|
||||
// skip the idle wait and flush pending results synchronously so we can
|
||||
// release the session lock ASAP.
|
||||
aborted: cleanupAbortLike,
|
||||
abortSettlePromise: cleanupAborted ? buildAbortSettlePromise() : null,
|
||||
skipSessionFlush: sessionLockController.hasSessionTakeover(),
|
||||
runId: params.runId,
|
||||
|
||||
Reference in New Issue
Block a user