From 0a042f68dfbe17f82b442d56f35bc83d70ca59b1 Mon Sep 17 00:00:00 2001 From: Peter Lee Date: Wed, 24 Jun 2026 18:50:58 -0500 Subject: [PATCH] fix(agent): replace self-wait with deferred release in retained-lock abort cleanup (#96100) * fix(agent): wait for retained session write before releasing held lock on abort * fix(agent): replace self-wait with deferred release in retained-lock abort cleanup * fix(test): reject fallback acquire with SessionWriteLockTimeoutError in active-scope cleanup test * fix(agent): trim retained-lock comments Signed-off-by: sallyom --------- Signed-off-by: sallyom Co-authored-by: sallyom --- .../run/attempt.session-lock.test.ts | 97 +++++++++++++++++++ .../run/attempt.session-lock.ts | 13 +++ 2 files changed, 110 insertions(+) diff --git a/src/agents/embedded-agent-runner/run/attempt.session-lock.test.ts b/src/agents/embedded-agent-runner/run/attempt.session-lock.test.ts index 642d72406372..e95c58280e0b 100644 --- a/src/agents/embedded-agent-runner/run/attempt.session-lock.test.ts +++ b/src/agents/embedded-agent-runner/run/attempt.session-lock.test.ts @@ -3992,6 +3992,103 @@ describe("embedded attempt session lock lifecycle", () => { expect(acquireSessionWriteLockLocal).toHaveBeenCalledTimes(2); }); + it("releaseHeldLockWithFence sets deferred flag when bailed out during active scope; re-attempted after scope deactivation (#95915)", async () => { + const events: string[] = []; + const releasePrep = vi.fn(async () => events.push("prep-release")); + const releaseRetained = vi.fn(async () => events.push("retained-release")); + const acquireSessionWriteLockLocal = vi + .fn() + .mockResolvedValueOnce({ release: releasePrep }) + .mockResolvedValueOnce({ release: releaseRetained }); + + const controller = await createEmbeddedAttemptSessionLockController({ + acquireSessionWriteLock: acquireSessionWriteLockLocal, + lockOptions, + }); + + await controller.releaseForPrompt(); + await controller.reacquireAfterPrompt(); + + await controller.withSessionWriteLock(async () => { + events.push("write-start"); + await controller.releaseHeldLockForAbort(); + events.push("write-end"); + }); + + expect(events).toEqual(["prep-release", "write-start", "write-end", "retained-release"]); + expect(acquireSessionWriteLockLocal).toHaveBeenCalledTimes(2); + }); + + it("controls the held lock lifecycle across deferred abort release, reacquisition, and prompt release", async () => { + const events: string[] = []; + const acquireSessionWriteLockLocal = vi + .fn() + .mockResolvedValueOnce({ release: vi.fn(async () => events.push("init-release")) }) + .mockResolvedValueOnce({ release: vi.fn(async () => events.push("held-release")) }) + .mockResolvedValueOnce({ release: vi.fn(async () => events.push("reacquire-release")) }); + + const controller = await createEmbeddedAttemptSessionLockController({ + acquireSessionWriteLock: acquireSessionWriteLockLocal, + lockOptions, + }); + + await controller.releaseForPrompt(); + await controller.reacquireAfterPrompt(); + + await controller.withSessionWriteLock(async () => { + events.push("write"); + await controller.releaseHeldLockForAbort(); + }); + + expect(events).toEqual(["init-release", "write", "held-release"]); + + await controller.reacquireAfterPrompt(); + await controller.releaseForPrompt(); + + expect(events).toEqual(["init-release", "write", "held-release", "reacquire-release"]); + expect(acquireSessionWriteLockLocal).toHaveBeenCalledTimes(3); + }); + + it("takeHeldLockAfterRetainedIdle does not self-deadlock when called from inside active write scope (#95915)", async () => { + const events: string[] = []; + const acquireSessionWriteLockLocal = vi + .fn() + .mockResolvedValueOnce({ release: vi.fn(async () => events.push("init-release")) }) + .mockResolvedValueOnce({ release: vi.fn(async () => events.push("held-release")) }) + .mockRejectedValueOnce( + new SessionWriteLockTimeoutError({ + timeoutMs: lockOptions.timeoutMs, + owner: "pid=test", + lockPath: `${lockOptions.sessionFile}.lock`, + }), + ); + + const controller = await createEmbeddedAttemptSessionLockController({ + acquireSessionWriteLock: acquireSessionWriteLockLocal, + lockOptions, + }); + + await controller.releaseForPrompt(); + await controller.reacquireAfterPrompt(); + + const takeoverError = await controller + .withSessionWriteLock(async () => { + events.push("write-start"); + const cleanupLock = await controller.acquireForCleanup(); + await cleanupLock.release(); + events.push("cleanup-inside-done"); + }) + .catch((error: unknown) => error); + + expect(takeoverError).toBeInstanceOf(EmbeddedAttemptSessionTakeoverError); + + const cleanupLock = await controller.acquireForCleanup(); + await cleanupLock.release(); + + expect(events).toEqual(["init-release", "write-start", "cleanup-inside-done", "held-release"]); + expect(acquireSessionWriteLockLocal).toHaveBeenCalledTimes(3); + }); + it("returns a no-op cleanup lock after prompt lock reacquisition times out", async () => { const releases: string[] = []; const acquireSessionWriteLockResult = vi diff --git a/src/agents/embedded-agent-runner/run/attempt.session-lock.ts b/src/agents/embedded-agent-runner/run/attempt.session-lock.ts index 449373f23dbe..d35b409d136f 100644 --- a/src/agents/embedded-agent-runner/run/attempt.session-lock.ts +++ b/src/agents/embedded-agent-runner/run/attempt.session-lock.ts @@ -1171,6 +1171,9 @@ export async function createEmbeddedAttemptSessionLockController(params: { let fenceGeneration = 0; let fenceActive = false; let takeoverDetected = false; + // Set when an active retained write prevents immediate held-lock release. + // The scope completion path retries release after the retained use unwinds. + let releaseHeldLockDeferred = false; let retainedLockUseCount = 0; const retainedLockIdleWaiters = new Set<() => void>(); let heldLockDraining = false; @@ -1603,6 +1606,7 @@ export async function createEmbeddedAttemptSessionLockController(params: { const drainOwner = await beginHeldLockDrain(); try { if (!(await waitForRetainedLockIdle())) { + releaseHeldLockDeferred = true; return; } if (!heldLock) { @@ -1639,6 +1643,8 @@ export async function createEmbeddedAttemptSessionLockController(params: { const drainOwner = await beginHeldLockDrain(); try { if (!(await waitForRetainedLockIdle())) { + // Do not wait for retained idle from inside the active scope; that + // scope must unwind before the retained-use waiter can resolve. return undefined; } if (!heldLock) { @@ -1660,6 +1666,7 @@ export async function createEmbeddedAttemptSessionLockController(params: { const drainOwner = await beginHeldLockDrain(); try { if (!(await waitForRetainedLockIdle())) { + // Same active-scope self-deadlock guard as takeHeldLockAfterRetainedIdle. return; } if (!heldLock) { @@ -1721,6 +1728,12 @@ export async function createEmbeddedAttemptSessionLockController(params: { } } await releaseHeldLockAfterTakeover(); + // Retained use has been released and the active scope is no longer live, + // so a prior active-scope release bailout can drain the held file lock now. + if (releaseHeldLockDeferred) { + releaseHeldLockDeferred = false; + await releaseHeldLockWithFence(); + } if (!outcome.ok) { throw outcome.error; }