mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-30 07:51:18 +00:00
fix(agents): scope compaction lock reentry to logical writer (#88919)
* fix: allow preflight compaction to reenter session locks * chore: refresh proof checks * test(agents): cover compaction lock reentry Replace the source-text guard with behavior-level coverage through the compaction harness. Co-authored-by: Plexus Technology <plexusadmin@plxsai.localdomain> * fix(agents): scope compaction lock reentry Reuse only the matching logical transcript writer lock and keep unrelated same-process compaction serialized. Co-authored-by: Plexus Technology <plexusadmin@plxsai.localdomain> * test(agents): preserve lock runner generics Co-authored-by: Plexus Technology <plexusadmin@plxsai.localdomain> * test(agents): preserve generic lock wrapper type Co-authored-by: Plexus Technology <plexusadmin@plxsai.localdomain> --------- Co-authored-by: Plexus Technology <plexusadmin@plxsai.localdomain> Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
@@ -48,6 +48,9 @@ export const hookRunner = {
|
||||
};
|
||||
|
||||
export const ensureRuntimePluginsLoaded: Mock<(params?: unknown) => void> = vi.fn();
|
||||
export const acquireSessionWriteLockMock = vi.fn(async (_params?: unknown) => ({
|
||||
release: vi.fn(async () => {}),
|
||||
}));
|
||||
export const resolveContextEngineMock = vi.fn(async () => ({
|
||||
info: { ownsCompaction: true as boolean },
|
||||
compact: contextEngineCompactMock,
|
||||
@@ -521,6 +524,7 @@ export function resetCompactHooksHarnessMocks(): void {
|
||||
hookRunner.runAfterCompaction.mockResolvedValue(undefined);
|
||||
|
||||
ensureRuntimePluginsLoaded.mockReset();
|
||||
acquireSessionWriteLockMock.mockClear();
|
||||
|
||||
resolveContextEngineMock.mockReset();
|
||||
resolveContextEngineMock.mockResolvedValue({
|
||||
@@ -587,6 +591,7 @@ export async function loadCompactHooksHarness(): Promise<{
|
||||
compactEmbeddedAgentSessionDirect: typeof import("./compact.js").compactEmbeddedAgentSessionDirect;
|
||||
compactEmbeddedAgentSession: typeof import("./compact.queued.js").compactEmbeddedAgentSession;
|
||||
testing: typeof import("./compact.js").testing;
|
||||
withOwnedSessionTranscriptWrites: typeof import("../../config/sessions/transcript-write-context.js").withOwnedSessionTranscriptWrites;
|
||||
onSessionTranscriptUpdate: typeof import("../../sessions/transcript-events.js").onSessionTranscriptUpdate;
|
||||
onInternalSessionTranscriptUpdate: typeof import("../../sessions/transcript-events.js").onInternalSessionTranscriptUpdate;
|
||||
}> {
|
||||
@@ -744,7 +749,7 @@ export async function loadCompactHooksHarness(): Promise<{
|
||||
}));
|
||||
|
||||
vi.doMock("../session-write-lock.js", () => ({
|
||||
acquireSessionWriteLock: vi.fn(async () => ({ release: vi.fn(async () => {}) })),
|
||||
acquireSessionWriteLock: acquireSessionWriteLockMock,
|
||||
resolveSessionLockMaxHoldFromTimeout: vi.fn(() => 0),
|
||||
resolveSessionWriteLockAcquireTimeoutMs: vi.fn(() => 60_000),
|
||||
resolveSessionWriteLockOptions: vi.fn(() => ({
|
||||
@@ -1049,17 +1054,20 @@ export async function loadCompactHooksHarness(): Promise<{
|
||||
};
|
||||
});
|
||||
|
||||
const [compactModule, compactQueuedModule, transcriptEvents] = await Promise.all([
|
||||
import("./compact.js"),
|
||||
import("./compact.queued.js"),
|
||||
import("../../sessions/transcript-events.js"),
|
||||
]);
|
||||
const [compactModule, compactQueuedModule, transcriptEvents, transcriptWriteContext] =
|
||||
await Promise.all([
|
||||
import("./compact.js"),
|
||||
import("./compact.queued.js"),
|
||||
import("../../sessions/transcript-events.js"),
|
||||
import("../../config/sessions/transcript-write-context.js"),
|
||||
]);
|
||||
|
||||
return {
|
||||
...compactModule,
|
||||
compactEmbeddedAgentSession: compactQueuedModule.compactEmbeddedAgentSession,
|
||||
onSessionTranscriptUpdate: transcriptEvents.onSessionTranscriptUpdate,
|
||||
onInternalSessionTranscriptUpdate: transcriptEvents.onInternalSessionTranscriptUpdate,
|
||||
withOwnedSessionTranscriptWrites: transcriptWriteContext.withOwnedSessionTranscriptWrites,
|
||||
};
|
||||
}
|
||||
/* oxlint-disable max-lines -- TODO: split this grandfathered oversized file. */
|
||||
|
||||
@@ -5,6 +5,7 @@ import type { AgentMessage } from "openclaw/plugin-sdk/agent-core";
|
||||
import { beforeAll, beforeEach, describe, expect, it, vi, type Mock } from "vitest";
|
||||
import { createReplyOperation } from "../../auto-reply/reply/reply-run-registry.js";
|
||||
import {
|
||||
acquireSessionWriteLockMock,
|
||||
applyExtraParamsToAgentMock,
|
||||
applyAgentCompactionSettingsFromConfigMock,
|
||||
buildAgentRuntimePlanMock,
|
||||
@@ -61,6 +62,7 @@ let compactEmbeddedAgentSession: typeof import("./compact.queued.js").compactEmb
|
||||
let compactTesting: typeof import("./compact.js").testing;
|
||||
let onSessionTranscriptUpdate: typeof import("../../sessions/transcript-events.js").onSessionTranscriptUpdate;
|
||||
let onInternalSessionTranscriptUpdate: typeof import("../../sessions/transcript-events.js").onInternalSessionTranscriptUpdate;
|
||||
let withOwnedSessionTranscriptWrites: typeof import("../../config/sessions/transcript-write-context.js").withOwnedSessionTranscriptWrites;
|
||||
|
||||
const TEST_SESSION_ID = "session-1";
|
||||
const TEST_SESSION_KEY = "agent:main:session-1";
|
||||
@@ -307,6 +309,7 @@ beforeAll(async () => {
|
||||
compactTesting = loaded.testing;
|
||||
onSessionTranscriptUpdate = loaded.onSessionTranscriptUpdate;
|
||||
onInternalSessionTranscriptUpdate = loaded.onInternalSessionTranscriptUpdate;
|
||||
withOwnedSessionTranscriptWrites = loaded.withOwnedSessionTranscriptWrites;
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
@@ -331,6 +334,36 @@ describe("compactEmbeddedAgentSessionDirect hooks", () => {
|
||||
resetCompactSessionStateMocks();
|
||||
});
|
||||
|
||||
it("acquires the normal session lock without process-wide reentry", async () => {
|
||||
const result = await compactEmbeddedAgentSessionDirect(wrappedCompactionArgs());
|
||||
|
||||
expect(result).toMatchObject({ ok: true, compacted: true });
|
||||
const lockOptions = mockCallArg(acquireSessionWriteLockMock);
|
||||
expect(lockOptions).toMatchObject({ sessionFile: TEST_SESSION_FILE });
|
||||
expect(lockOptions).not.toHaveProperty("allowReentrant");
|
||||
});
|
||||
|
||||
it("reuses the matching logical writer lock during direct compaction", async () => {
|
||||
const withSessionWriteLockCall = vi.fn();
|
||||
const withSessionWriteLock = async <T>(run: () => Promise<T> | T): Promise<T> => {
|
||||
withSessionWriteLockCall();
|
||||
return await run();
|
||||
};
|
||||
|
||||
const result = await withOwnedSessionTranscriptWrites(
|
||||
{
|
||||
sessionFile: TEST_SESSION_FILE,
|
||||
sessionKey: TEST_SESSION_KEY,
|
||||
withSessionWriteLock,
|
||||
},
|
||||
async () => await compactEmbeddedAgentSessionDirect(wrappedCompactionArgs()),
|
||||
);
|
||||
|
||||
expect(result).toMatchObject({ ok: true, compacted: true });
|
||||
expect(withSessionWriteLockCall).toHaveBeenCalledOnce();
|
||||
expect(acquireSessionWriteLockMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("fails closed before generic compaction for a model-locked native session", async () => {
|
||||
const result = await compactEmbeddedAgentSessionDirect({
|
||||
sessionId: "session-1",
|
||||
|
||||
@@ -7,6 +7,7 @@ import { isAcpRuntimeSpawnAvailable } from "../../acp/runtime/availability.js";
|
||||
import type { ThinkLevel } from "../../auto-reply/thinking.js";
|
||||
import { resolveAgentModelFallbackValues } from "../../config/model-input.js";
|
||||
import { parseSqliteSessionFileMarker } from "../../config/sessions/sqlite-marker.js";
|
||||
import { acquireOwnedSessionTranscriptWriteLock } from "../../config/sessions/transcript-write-context.js";
|
||||
import type { OpenClawConfig } from "../../config/types.openclaw.js";
|
||||
import {
|
||||
createFileBackedCompactionCheckpointStore,
|
||||
@@ -1376,14 +1377,19 @@ async function compactEmbeddedAgentSessionDirectOnce(
|
||||
};
|
||||
|
||||
const compactionTimeoutMs = resolveCompactionTimeoutMs(params.config);
|
||||
const sessionLock = await acquireSessionWriteLock({
|
||||
sessionFile: params.sessionFile,
|
||||
...resolveSessionWriteLockOptions(params.config, {
|
||||
maxHoldMsFallback: resolveSessionLockMaxHoldFromTimeout({
|
||||
timeoutMs: compactionTimeoutMs,
|
||||
const sessionLock =
|
||||
(await acquireOwnedSessionTranscriptWriteLock({
|
||||
sessionFile: params.sessionFile,
|
||||
sessionKey: params.sessionKey,
|
||||
})) ??
|
||||
(await acquireSessionWriteLock({
|
||||
sessionFile: params.sessionFile,
|
||||
...resolveSessionWriteLockOptions(params.config, {
|
||||
maxHoldMsFallback: resolveSessionLockMaxHoldFromTimeout({
|
||||
timeoutMs: compactionTimeoutMs,
|
||||
}),
|
||||
}),
|
||||
}),
|
||||
});
|
||||
}));
|
||||
try {
|
||||
if (!isSqliteSessionTranscript) {
|
||||
await repairSessionFileIfNeeded({
|
||||
|
||||
@@ -87,6 +87,47 @@ export async function runWithOwnedSessionTranscriptWriteLock<T>(
|
||||
return await runWithOwnedSessionTranscriptWriteContext(params, run);
|
||||
}
|
||||
|
||||
export async function acquireOwnedSessionTranscriptWriteLock(params: {
|
||||
sessionFile?: string;
|
||||
sessionKey?: string;
|
||||
}): Promise<{ release: () => Promise<void> } | undefined> {
|
||||
const context = ownedTranscriptWriteContext.getStore();
|
||||
if (!context || !contextMatches({ context, ...params })) {
|
||||
return undefined;
|
||||
}
|
||||
|
||||
// Keep the owner callback pending until release so release-shaped callers
|
||||
// cannot outlive the logical writer lock or leak a tracked nested operation.
|
||||
let markAcquired!: () => void;
|
||||
let rejectAcquire!: (error: unknown) => void;
|
||||
const acquired = new Promise<void>((resolve, reject) => {
|
||||
markAcquired = resolve;
|
||||
rejectAcquire = reject;
|
||||
});
|
||||
let releaseOperation!: () => void;
|
||||
const releaseRequested = new Promise<void>((resolve) => {
|
||||
releaseOperation = resolve;
|
||||
});
|
||||
const operation = context.withSessionWriteLock(async () => {
|
||||
markAcquired();
|
||||
await releaseRequested;
|
||||
});
|
||||
void operation.catch(rejectAcquire);
|
||||
await acquired;
|
||||
|
||||
let released = false;
|
||||
return {
|
||||
release: async () => {
|
||||
if (released) {
|
||||
return;
|
||||
}
|
||||
released = true;
|
||||
releaseOperation();
|
||||
await operation;
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
export function canAdvanceOwnedSessionEntryCache(params: {
|
||||
sessionFile?: string;
|
||||
sessionKey?: string;
|
||||
|
||||
Reference in New Issue
Block a user