fix(agent): trim retained-lock comments

sallyom · sallyom · commit 457aa1ebbcca · 2026-06-24T18:14:45.000-04:00
Signed-off-by: sallyom &lt;somalley@redhat.com&gt;
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
@@ -3993,11 +3993,6 @@ describe("embedded attempt session lock lifecycle", () => {
   });
 
   it("releaseHeldLockWithFence sets deferred flag when bailed out during active scope; re-attempted after scope deactivation (#95915)", async () => {
-    // When releaseHeldLockForAbort is called from inside an active write
-    // scope, releaseHeldLockWithFence bails out (waitForRetainedLockIdle
-    // returns false) and sets releaseHeldLockDeferred. After the write scope
-    // completes, runWithPhysicalWriteLockScope must re-attempt the release
-    // so the held lock does not leak until the watchdog fires.
     const events: string[] = [];
     const releasePrep = vi.fn(async () => events.push("prep-release"));
     const releaseRetained = vi.fn(async () => events.push("retained-release"));
@@ -4011,36 +4006,20 @@ describe("embedded attempt session lock lifecycle", () => {
       lockOptions,
     });
 
-    // Construction acquired a lock (releasePrep). Release and reacquire
-    // to set up the held lock (releaseRetained) that write scopes share.
     await controller.releaseForPrompt();
     await controller.reacquireAfterPrompt();
 
-    // Call releaseHeldLockForAbort from inside the active write scope.
-    // releaseHeldLockWithFence detects the active scope, sets
-    // releaseHeldLockDeferred, and returns without releasing.
     await controller.withSessionWriteLock(async () => {
       events.push("write-start");
       await controller.releaseHeldLockForAbort();
       events.push("write-end");
     });
 
-    // After the write scope completes, runWithPhysicalWriteLockScope
-    // sees releaseHeldLockDeferred and re-attempts the release. The held
-    // lock (releaseRetained) should now be released.
-    expect(events).toEqual([
-      "prep-release", // releaseForPrompt released the initial lock
-      "write-start",
-      "write-end",
-      "retained-release", // deferred releaseHeldLockWithFence at scope completion
-    ]);
+    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 () => {
-    // Integration test: deferred abort release → reacquire → prompt release.
-    // Verifies that after a deferred releaseHeldLockWithFence, the lock
-    // state is clean and subsequent lock operations work correctly.
     const events: string[] = [];
     const acquireSessionWriteLockLocal = vi
       .fn()
@@ -4053,35 +4032,24 @@ describe("embedded attempt session lock lifecycle", () => {
       lockOptions,
     });
 
-    // Release initial and reacquire to set up held lock.
     await controller.releaseForPrompt();
     await controller.reacquireAfterPrompt();
 
-    // Abort from inside active scope → deferred release.
     await controller.withSessionWriteLock(async () => {
       events.push("write");
       await controller.releaseHeldLockForAbort();
     });
 
     expect(events).toEqual(["init-release", "write", "held-release"]);
 
-    // After deferred release, reacquire a fresh held lock.
     await controller.reacquireAfterPrompt();
-    // Prompt release should release the fresh lock.
     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 () => {
-    // When acquireForCleanup is called from within an active retained write
-    // scope, takeHeldLockAfterRetainedIdle must NOT await
-    // retainedLockIdleWaiters — the waiter resolves only when the
-    // retained use count reaches zero, which cannot happen while the
-    // current active scope is still alive. Instead it returns undefined
-    // (scope active, can't take lock). acquireCleanupLock falls through to
-    // acquireLock which fails with EEXIST → returns undefined → noopLock.
     const events: string[] = [];
     const acquireSessionWriteLockLocal = vi
       .fn()
@@ -4100,35 +4068,20 @@ describe("embedded attempt session lock lifecycle", () => {
       lockOptions,
     });
 
-    // Release initial lock and reacquire to set up held lock.
     await controller.releaseForPrompt();
     await controller.reacquireAfterPrompt();
 
-    // Call acquireForCleanup from inside the active write scope.
-    // This must not deadlock. The fallback acquireLock rejects with
-    // SessionWriteLockTimeoutError (same-pid lock-timeout branch),
-    // acquireCleanupLock catches it via isSessionWriteLockAcquireError
-    // and returns undefined → noopLock. The scope then detects
-    // takeoverDetected and throws EmbeddedAttemptSessionTakeoverError.
     const takeoverError = await controller
       .withSessionWriteLock(async () => {
         events.push("write-start");
-        // Calling acquireForCleanup inside the active scope exercises
-        // takeHeldLockAfterRetainedIdle's false branch. It must return
-        // undefined (not await retainedLockIdleWaiters).
         const cleanupLock = await controller.acquireForCleanup();
-        // NoopLock — release is a safe no-op.
         await cleanupLock.release();
         events.push("cleanup-inside-done");
       })
       .catch((error: unknown) => error);
 
     expect(takeoverError).toBeInstanceOf(EmbeddedAttemptSessionTakeoverError);
 
-    // After the scope throws with takeover, the held lock is still set
-    // (takeHeldLockAfterRetainedIdle inside the scope returned undefined
-    // and the deferred release path was not triggered). A real cleanup
-    // call outside the scope can take the held lock.
     const cleanupLock = await controller.acquireForCleanup();
     await cleanupLock.release();
 
diff --git a/src/agents/embedded-agent-runner/run/attempt.session-lock.ts b/src/agents/embedded-agent-runner/run/attempt.session-lock.ts
@@ -1171,10 +1171,8 @@ export async function createEmbeddedAttemptSessionLockController(params: {
   let fenceGeneration = 0;
   let fenceActive = false;
   let takeoverDetected = false;
-  // Tracks whether releaseHeldLockWithFence() bailed out because the active
-  // write-lock scope is still alive. When set, runWithPhysicalWriteLockScope
-  // re-attempts the release after the scope is deactivated so the held lock
-  // does not leak until the watchdog fires.
+  // 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>();
@@ -1608,10 +1606,6 @@ export async function createEmbeddedAttemptSessionLockController(params: {
     const drainOwner = await beginHeldLockDrain();
     try {
       if (!(await waitForRetainedLockIdle())) {
-        // A retained session write is in progress and the caller is inside
-        // the active write-lock scope. Defer the release: when the write
-        // scope completes, runWithPhysicalWriteLockScope re-attempts
-        // releaseHeldLockWithFence after scope deactivation.
         releaseHeldLockDeferred = true;
         return;
       }
@@ -1649,16 +1643,8 @@ export async function createEmbeddedAttemptSessionLockController(params: {
     const drainOwner = await beginHeldLockDrain();
     try {
       if (!(await waitForRetainedLockIdle())) {
-        // A retained session write is in progress and the caller is inside the
-        // active write-lock scope. Do NOT await retainedLockIdleWaiters here —
-        // that waiter resolves when the retained use count reaches zero, which
-        // cannot happen until the current active scope unwinds. Awaiting from
-        // inside that same scope would self-deadlock.
-        //
-        // Return undefined so acquireCleanupLock() falls through to
-        // acquireLock(). The held lock will be released by the scope-completion
-        // path in runWithPhysicalWriteLockScope, which re-attempts
-        // releaseHeldLockWithFence() when releaseHeldLockDeferred is set.
+        // 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) {
@@ -1680,11 +1666,7 @@ export async function createEmbeddedAttemptSessionLockController(params: {
     const drainOwner = await beginHeldLockDrain();
     try {
       if (!(await waitForRetainedLockIdle())) {
-        // A retained session write is in progress and the caller is inside the
-        // active write-lock scope. Do NOT await retainedLockIdleWaiters here —
-        // same self-deadlock risk described in takeHeldLockAfterRetainedIdle.
-        // The held lock will be released by the scope-completion path in
-        // runWithPhysicalWriteLockScope when releaseHeldLockDeferred is set.
+        // Same active-scope self-deadlock guard as takeHeldLockAfterRetainedIdle.
         return;
       }
       if (!heldLock) {
@@ -1746,16 +1728,8 @@ export async function createEmbeddedAttemptSessionLockController(params: {
       }
     }
     await releaseHeldLockAfterTakeover();
-    // If releaseHeldLockWithFence() bailed out earlier because the active
-    // write-lock scope was still alive, re-attempt the release now that the
-    // scope has been deactivated and the retained-use count has been released.
-    // At this point waitForRetainedLockIdle() will not self-wait because the
-    // current scope is no longer the active one.
-    //
-    // This fixes a lock-leak scenario where abort fires during a retained
-    // session write: releaseHeldLockWithFence bails out (scope active), leaving
-    // heldLock set. Without this re-attempt the held lock would only be released
-    // by the maxHoldMs watchdog (issue #95915).
+    // 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();
