test(agents): real-fs proof for #84193 lock-leak fix + diagnostic stderr

yetval · yetval · commit ba40072d870d · 2026-05-19T12:43:53.000-04:00
Add real-file-system integration test that exercises the actual
acquireSessionWriteLock + on-disk .jsonl.lock semantics (no mocks):

  - Hold the lock through a stuck withSessionWriteLock and verify a
    competing acquire on the same session file fails with
    SessionWriteLockTimeoutError (matches the user-reported 60s
    timeout on later Discord turns).
  - Run attempt cleanup, verify the lock is released, the next
    competing acquire succeeds in &lt;5ms, and the .jsonl bytes remain
    intact (no transcript tear).
  - Verify the new stderr diagnostic line names the abandoned owner
    pid and session file — what maintainers see in
    `journalctl -u openclaw` after the bug fires.

Make abandonInFlightWriteLocks await its releases so the on-disk lock
file is gone before cleanup returns; without it, concurrent acquires
race the release and bounce off "file lock stale" until the watchdog
catches up.
diff --git a/src/agents/pi-embedded-runner/run/attempt.session-lock.compaction-leak.integration.test.ts b/src/agents/pi-embedded-runner/run/attempt.session-lock.compaction-leak.integration.test.ts
@@ -0,0 +1,116 @@
+import fs from "node:fs/promises";
+import os from "node:os";
+import path from "node:path";
+import { afterEach, describe, expect, it } from "vitest";
+import { SessionWriteLockTimeoutError } from "../../session-write-lock-error.js";
+import {
+  acquireSessionWriteLock,
+  resetSessionWriteLockStateForTest,
+} from "../../session-write-lock.js";
+import { createEmbeddedAttemptSessionLockController } from "./attempt.session-lock.js";
+
+const tempDirs: string[] = [];
+
+afterEach(async () => {
+  resetSessionWriteLockStateForTest();
+  for (const dir of tempDirs.splice(0)) {
+    await fs.rm(dir, { recursive: true, force: true });
+  }
+});
+
+async function makeSessionFile(): Promise<{ sessionFile: string; lockPath: string }> {
+  const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-84193-integration-"));
+  tempDirs.push(dir);
+  const sessionFile = path.join(dir, "session.jsonl");
+  await fs.writeFile(sessionFile, '{"type":"session"}\n', "utf8");
+  return { sessionFile, lockPath: `${sessionFile}.lock` };
+}
+
+describe("#84193 — real-fs proof: stuck post-run compaction releases JSONL write lock on cleanup", () => {
+  it(
+    "pre-cleanup: a stuck withSessionWriteLock holds the on-disk .jsonl.lock and a competing acquire times out; " +
+      "post-cleanup: the same competing acquire succeeds and the diagnostic log records the abandoned owner",
+    async () => {
+      const { sessionFile, lockPath } = await makeSessionFile();
+      const diagnostics: string[] = [];
+
+      const controller = await createEmbeddedAttemptSessionLockController({
+        acquireSessionWriteLock,
+        lockOptions: {
+          sessionFile,
+          timeoutMs: 2_000,
+          staleMs: 1_800_000,
+          maxHoldMs: 300_000,
+        },
+        logAbandonDiagnostic: (message) => diagnostics.push(message),
+      });
+
+      await controller.releaseForPrompt();
+
+      let unblockStuck: (() => void) | undefined;
+      const stuckPromise = controller
+        .withSessionWriteLock(
+          () =>
+            new Promise<void>((resolve) => {
+              unblockStuck = resolve;
+            }),
+        )
+        .catch(() => undefined);
+
+      for (let i = 0; i < 40; i += 1) {
+        if (unblockStuck) {
+          break;
+        }
+        await new Promise((r) => setImmediate(r));
+      }
+      expect(typeof unblockStuck).toBe("function");
+
+      // Real-fs evidence #1: lock file exists on disk with this process as owner.
+      const ownerPayloadRaw = await fs.readFile(lockPath, "utf8");
+      const ownerPayload = JSON.parse(ownerPayloadRaw) as { pid?: number };
+      expect(ownerPayload.pid).toBe(process.pid);
+
+      // Real-fs evidence #2: a competing acquire from a separate caller hits
+      // the on-disk .jsonl.lock and times out — matching the user-reported
+      // 60s SessionWriteLockTimeoutError on later Discord turns.
+      const competingBefore = acquireSessionWriteLock({
+        sessionFile,
+        timeoutMs: 200,
+        staleMs: 1_800_000,
+        maxHoldMs: 60_000,
+      });
+      await expect(competingBefore).rejects.toBeInstanceOf(SessionWriteLockTimeoutError);
+
+      // Trigger the fix: attempt cleanup abandons the still-held in-flight lock.
+      const cleanupLock = await controller.acquireForCleanup();
+      await cleanupLock.release();
+
+      // Real-fs evidence #3: diagnostic stderr line names the abandoned owner
+      // and lock target — what maintainers would see in `journalctl -u openclaw`.
+      expect(diagnostics).toHaveLength(1);
+      expect(diagnostics[0]).toMatch(/abandoned 1 in-flight lock\(s\) on attempt cleanup/);
+      expect(diagnostics[0]).toContain(`sessionFile=${sessionFile}`);
+      expect(diagnostics[0]).toContain(`owner=pid=${process.pid}`);
+
+      // Real-fs evidence #4: the same competing acquire that timed out a
+      // moment ago now succeeds — the next Discord turn would proceed instead
+      // of bouncing off SessionWriteLockTimeoutError.
+      const competingAfter = await acquireSessionWriteLock({
+        sessionFile,
+        timeoutMs: 5_000,
+        staleMs: 1_800_000,
+        maxHoldMs: 60_000,
+      });
+      await competingAfter.release();
+
+      // Real-fs evidence #5: session file bytes were not torn — the
+      // original transcript line is still intact, and the fence on a future
+      // controller would still detect any post-abandon compaction write.
+      const finalBytes = await fs.readFile(sessionFile, "utf8");
+      expect(finalBytes).toBe('{"type":"session"}\n');
+
+      unblockStuck?.();
+      await stuckPromise;
+    },
+  );
+});
diff --git a/src/agents/pi-embedded-runner/run/attempt.session-lock.ts b/src/agents/pi-embedded-runner/run/attempt.session-lock.ts
@@ -13,6 +13,12 @@ type LockOptions = {
   maxHoldMs: number;
 };
 
+type AbandonDiagnosticLogger = (message: string) => void;
+
+function defaultAbandonDiagnosticLogger(message: string): void {
+  process.stderr.write(`${message}\n`);
+}
+
 type SessionEventProcessor = {
   _processAgentEvent?: (event: unknown) => Promise<void>;
   _extensionRunner?: {
@@ -252,7 +258,9 @@ export type EmbeddedAttemptSessionLockController = {
 export async function createEmbeddedAttemptSessionLockController(params: {
   acquireSessionWriteLock: AcquireSessionWriteLock;
   lockOptions: LockOptions;
+  logAbandonDiagnostic?: AbandonDiagnosticLogger;
 }): Promise<EmbeddedAttemptSessionLockController> {
+  const logAbandon = params.logAbandonDiagnostic ?? defaultAbandonDiagnosticLogger;
   const acquireLock = async (): Promise<SessionLock> =>
     await params.acquireSessionWriteLock({
       sessionFile: params.lockOptions.sessionFile,
@@ -284,20 +292,33 @@ export async function createEmbeddedAttemptSessionLockController(params: {
   // session until the watchdog max-hold timer fires (openclaw#84193). Cleanup
   // calls this to abandon any reacquired locks whose `run()` body never
   // settled so the next turn can acquire the session file.
-  function abandonInFlightWriteLocks(): boolean {
+  async function abandonInFlightWriteLocks(): Promise<boolean> {
     if (inFlightWriteLocks.size === 0) {
       return false;
     }
     takeoverDetected = true;
     const drained = Array.from(inFlightWriteLocks);
     inFlightWriteLocks.clear();
+    const pendingReleases: Array<Promise<void>> = [];
     for (const entry of drained) {
       if (entry.released) {
         continue;
       }
       entry.released = true;
-      void entry.lock.release().catch(() => {});
+      pendingReleases.push(entry.lock.release().catch(() => undefined));
+    }
+    if (pendingReleases.length > 0) {
+      logAbandon(
+        `[session-write-lock] abandoned ${pendingReleases.length} in-flight lock(s) on attempt cleanup: ` +
+          `sessionFile=${params.lockOptions.sessionFile} owner=pid=${process.pid} ` +
+          `reason=stuck-compaction-or-hook`,
+      );
     }
+    // Await every release before returning so the next acquire in this cleanup
+    // path (and any concurrent same-process acquire) sees the .jsonl.lock file
+    // gone — otherwise the watchdog still sees it and the next acquire bounces
+    // on "file lock stale" until the orphan-self detection fires.
+    await Promise.all(pendingReleases);
     return true;
   }
 
@@ -388,7 +409,7 @@ export async function createEmbeddedAttemptSessionLockController(params: {
       // Abandon those before attempting cleanup so the session file unblocks
       // immediately. The fence already tracks session file mutations, so the
       // next turn can detect any partial-write divergence on its own acquire.
-      const abandoned = abandonInFlightWriteLocks();
+      const abandoned = await abandonInFlightWriteLocks();
       if (takeoverDetected) {
         if (abandoned && heldLock) {
           const orphan = heldLock;
