Skip to content

Commit be33b90

Browse files
ubeheraubehera
authored
Merge 2606e52 into 4424daf 
2 parents 4424daf + 2606e52 commit be33b90

3 files changed

Lines changed: 402 additions & 7 deletions

File tree

‎src/agents/pi-embedded-runner/run/attempt.session-lock.test.ts‎

Lines changed: 267 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {
1919
installPromptSubmissionLockRelease,
2020
installSessionEventWriteLock,
2121
installSessionExternalHookWriteLock,
22+
resetEmbeddedAttemptSessionFilePromptGuardsForTest,
2223
} from "./attempt.session-lock.js";
2324

2425
const lockOptions = {
@@ -32,6 +33,7 @@ const tempDirs: string[] = [];
3233

3334
afterEach(async () => {
3435
resetSessionWriteLockStateForTest();
36+
resetEmbeddedAttemptSessionFilePromptGuardsForTest();
3537
for (const dir of tempDirs.splice(0)) {
3638
await fs.rm(dir, { recursive: true, force: true });
3739
}
@@ -531,6 +533,7 @@ describe("embedded attempt session lock lifecycle", () => {
531533
},
532534
),
533535
);
536+
resetEmbeddedAttemptSessionFilePromptGuardsForTest();
534537
await secondController.releaseForPrompt();
535538

536539
await expect(
@@ -564,6 +567,7 @@ describe("embedded attempt session lock lifecycle", () => {
564567
acquireSessionWriteLock,
565568
lockOptions: { ...lockOptions, sessionFile },
566569
});
570+
resetEmbeddedAttemptSessionFilePromptGuardsForTest();
567571
await secondController.releaseForPrompt();
568572
const cleanupLock = await secondController.acquireForCleanup();
569573

@@ -629,6 +633,7 @@ describe("embedded attempt session lock lifecycle", () => {
629633
},
630634
),
631635
);
636+
resetEmbeddedAttemptSessionFilePromptGuardsForTest();
632637
await secondController.releaseForPrompt();
633638

634639
await expect(
@@ -665,6 +670,7 @@ describe("embedded attempt session lock lifecycle", () => {
665670
await fs.appendFile(sessionFile, '{"type":"message","id":"same-process"}\n', "utf8");
666671
await fs.appendFile(sessionFile, '{"type":"message","id":"external-interleaved"}\n', "utf8");
667672
});
673+
resetEmbeddedAttemptSessionFilePromptGuardsForTest();
668674
await secondController.releaseForPrompt();
669675

670676
await expect(
@@ -698,6 +704,7 @@ describe("embedded attempt session lock lifecycle", () => {
698704
acquireSessionWriteLock,
699705
lockOptions: { ...lockOptions, sessionFile },
700706
});
707+
resetEmbeddedAttemptSessionFilePromptGuardsForTest();
701708
await secondController.releaseForPrompt();
702709

703710
await expect(
@@ -734,6 +741,7 @@ describe("embedded attempt session lock lifecycle", () => {
734741
await secondController.withSessionWriteLock(async () => {
735742
await fs.appendFile(sessionFile, '{"type":"message","id":"same-process"}\n', "utf8");
736743
});
744+
resetEmbeddedAttemptSessionFilePromptGuardsForTest();
737745
await secondController.releaseForPrompt();
738746

739747
await expect(
@@ -1205,4 +1213,263 @@ describe("embedded attempt session lock lifecycle", () => {
12051213

12061214
expect(events).toEqual(["queue-drained", "lock", "hook-start", "hook-end"]);
12071215
});
1216+
1217+
it("serialises prompt windows on the same session file across two controllers", async () => {
1218+
const sessionFile = await createTempSessionFile();
1219+
const events: string[] = [];
1220+
let acquireCount = 0;
1221+
const acquireSessionWriteLock = vi.fn(async () => {
1222+
acquireCount += 1;
1223+
const id = acquireCount;
1224+
events.push(`acquire-${id}`);
1225+
return {
1226+
release: vi.fn(async () => {
1227+
events.push(`release-${id}`);
1228+
}),
1229+
};
1230+
});
1231+
1232+
const firstController = await createEmbeddedAttemptSessionLockController({
1233+
acquireSessionWriteLock,
1234+
lockOptions: { ...lockOptions, sessionFile },
1235+
});
1236+
const secondController = await createEmbeddedAttemptSessionLockController({
1237+
acquireSessionWriteLock,
1238+
lockOptions: { ...lockOptions, sessionFile },
1239+
});
1240+
1241+
await firstController.releaseForPrompt();
1242+
1243+
let secondCompletedRelease = false;
1244+
const secondReleasePromise = secondController.releaseForPrompt().then(() => {
1245+
secondCompletedRelease = true;
1246+
});
1247+
// Yield several microtasks; the second release must still be blocked
1248+
// because the first controller has not yet reacquired its prompt turn.
1249+
await Promise.resolve();
1250+
await Promise.resolve();
1251+
expect(secondCompletedRelease).toBe(false);
1252+
1253+
await firstController.reacquireAfterPrompt();
1254+
await secondReleasePromise;
1255+
1256+
expect(secondCompletedRelease).toBe(true);
1257+
expect(firstController.hasSessionTakeover()).toBe(false);
1258+
expect(secondController.hasSessionTakeover()).toBe(false);
1259+
});
1260+
1261+
it("vacates the prompt turn so later waiters proceed when reacquireAfterPrompt throws", async () => {
1262+
// Regression for the file-scoped guard's cleanup-on-failure path. If
1263+
// reacquireAfterPrompt's error branch did not release the turn, the
1264+
// next same-file waiter would block forever — i.e. one failed prompt
1265+
// window would wedge the session file for the rest of the process.
1266+
const sessionFile = await createTempSessionFile();
1267+
const events: string[] = [];
1268+
1269+
const firstInitialRelease = vi.fn(async () => {
1270+
events.push("first-initial-release");
1271+
});
1272+
const secondInitialRelease = vi.fn(async () => {
1273+
events.push("second-initial-release");
1274+
});
1275+
const secondReacquireRelease = vi.fn(async () => {
1276+
events.push("second-reacquire-release");
1277+
});
1278+
1279+
const acquireSessionWriteLock = vi.fn();
1280+
acquireSessionWriteLock
1281+
.mockResolvedValueOnce({ release: firstInitialRelease })
1282+
.mockResolvedValueOnce({ release: secondInitialRelease })
1283+
// First controller's reacquireAfterPrompt rejects with a real
1284+
// lock-timeout error (the same shape the production code would see if
1285+
// another process or lane held the lock past timeoutMs).
1286+
.mockRejectedValueOnce(
1287+
new SessionWriteLockTimeoutError({
1288+
timeoutMs: lockOptions.timeoutMs,
1289+
owner: "pid=external",
1290+
lockPath: `${sessionFile}.lock`,
1291+
}),
1292+
)
1293+
.mockResolvedValueOnce({ release: secondReacquireRelease });
1294+
1295+
const firstController = await createEmbeddedAttemptSessionLockController({
1296+
acquireSessionWriteLock,
1297+
lockOptions: { ...lockOptions, sessionFile },
1298+
});
1299+
const secondController = await createEmbeddedAttemptSessionLockController({
1300+
acquireSessionWriteLock,
1301+
lockOptions: { ...lockOptions, sessionFile },
1302+
});
1303+
1304+
await firstController.releaseForPrompt();
1305+
1306+
let secondCompletedRelease = false;
1307+
const secondReleasePromise = secondController.releaseForPrompt().then(() => {
1308+
secondCompletedRelease = true;
1309+
});
1310+
await Promise.resolve();
1311+
expect(secondCompletedRelease).toBe(false);
1312+
1313+
await expect(firstController.reacquireAfterPrompt()).rejects.toBeInstanceOf(
1314+
SessionWriteLockTimeoutError,
1315+
);
1316+
1317+
// The first turn must have been released by reacquireAfterPrompt's
1318+
// finally block, even though the body threw. The second waiter is now
1319+
// free to proceed through its own release-for-prompt.
1320+
await secondReleasePromise;
1321+
expect(secondCompletedRelease).toBe(true);
1322+
1323+
// The second controller observes a clean prompt window; the file-scoped
1324+
// queue did not get wedged by the first lane's failed reacquire.
1325+
expect(secondController.hasSessionTakeover()).toBe(false);
1326+
});
1327+
1328+
it("vacates the prompt turn when releaseForPrompt itself fails mid-flight", async () => {
1329+
// Regression for the same cleanup-on-failure path on the other side of
1330+
// the prompt window. If releaseForPrompt throws partway — for example
1331+
// the held lock's release rejects — the turn slot must still be
1332+
// vacated. Otherwise the next same-file waiter blocks forever even
1333+
// though no prompt is actually in flight.
1334+
const sessionFile = await createTempSessionFile();
1335+
1336+
const firstInitialRelease = vi.fn(async () => {
1337+
throw new Error("simulated lock-release failure");
1338+
});
1339+
const secondInitialRelease = vi.fn(async () => {});
1340+
const secondReacquireRelease = vi.fn(async () => {});
1341+
const thirdInitialRelease = vi.fn(async () => {});
1342+
1343+
const acquireSessionWriteLock = vi.fn();
1344+
acquireSessionWriteLock
1345+
// First and second start with their own locks; the second is
1346+
// blocked waiting on the first's turn at the point first throws.
1347+
.mockResolvedValueOnce({ release: firstInitialRelease })
1348+
.mockResolvedValueOnce({ release: secondInitialRelease })
1349+
// The second controller's prior-wait dance reacquires here after
1350+
// the first turn vacates on its release failure.
1351+
.mockResolvedValueOnce({ release: secondReacquireRelease })
1352+
.mockResolvedValueOnce({ release: thirdInitialRelease });
1353+
1354+
const firstController = await createEmbeddedAttemptSessionLockController({
1355+
acquireSessionWriteLock,
1356+
lockOptions: { ...lockOptions, sessionFile },
1357+
});
1358+
const secondController = await createEmbeddedAttemptSessionLockController({
1359+
acquireSessionWriteLock,
1360+
lockOptions: { ...lockOptions, sessionFile },
1361+
});
1362+
1363+
// First controller's releaseForPrompt now reaches lock.release() which
1364+
// rejects. The turn must be vacated by the catch block.
1365+
await expect(firstController.releaseForPrompt()).rejects.toThrow(
1366+
"simulated lock-release failure",
1367+
);
1368+
1369+
// Second controller's releaseForPrompt must complete without blocking
1370+
// on a dangling turn from the failed first release.
1371+
await secondController.releaseForPrompt();
1372+
expect(secondController.hasSessionTakeover()).toBe(false);
1373+
});
1374+
1375+
it("does not block prompt-window waiters behind a compaction-wait release on the same file", async () => {
1376+
// releaseForSessionIdleWait is for the post-prompt compaction window;
1377+
// it must not register a prompt-turn or other lanes will wait on it
1378+
// unnecessarily.
1379+
const sessionFile = await createTempSessionFile();
1380+
const events: string[] = [];
1381+
let acquireCount = 0;
1382+
const acquireSessionWriteLock = vi.fn(async () => {
1383+
acquireCount += 1;
1384+
const id = acquireCount;
1385+
events.push(`acquire-${id}`);
1386+
return {
1387+
release: vi.fn(async () => {
1388+
events.push(`release-${id}`);
1389+
}),
1390+
};
1391+
});
1392+
1393+
const firstController = await createEmbeddedAttemptSessionLockController({
1394+
acquireSessionWriteLock,
1395+
lockOptions: { ...lockOptions, sessionFile },
1396+
});
1397+
const secondController = await createEmbeddedAttemptSessionLockController({
1398+
acquireSessionWriteLock,
1399+
lockOptions: { ...lockOptions, sessionFile },
1400+
});
1401+
1402+
await firstController.releaseForSessionIdleWait();
1403+
1404+
// Second controller's prompt window should run without waiting on
1405+
// anything from the idle-wait release.
1406+
await secondController.releaseForPrompt();
1407+
1408+
expect(firstController.hasSessionTakeover()).toBe(false);
1409+
expect(secondController.hasSessionTakeover()).toBe(false);
1410+
});
1411+
1412+
it("does not serialise prompt windows across different session files", async () => {
1413+
const sessionFileA = await createTempSessionFile();
1414+
const sessionFileB = await createTempSessionFile();
1415+
const acquireSessionWriteLock = vi.fn(async () => ({
1416+
release: vi.fn(async () => {}),
1417+
}));
1418+
1419+
const controllerA = await createEmbeddedAttemptSessionLockController({
1420+
acquireSessionWriteLock,
1421+
lockOptions: { ...lockOptions, sessionFile: sessionFileA },
1422+
});
1423+
const controllerB = await createEmbeddedAttemptSessionLockController({
1424+
acquireSessionWriteLock,
1425+
lockOptions: { ...lockOptions, sessionFile: sessionFileB },
1426+
});
1427+
1428+
await controllerA.releaseForPrompt();
1429+
// B's prompt window targets a different session file — it must not
1430+
// block on A's outstanding turn.
1431+
await controllerB.releaseForPrompt();
1432+
expect(controllerB.hasSessionTakeover()).toBe(false);
1433+
});
1434+
1435+
it("dispose-after-releaseForPrompt vacates the prompt turn so the next same-file controller does not hang", async () => {
1436+
// Regression for the ClawSweeper P2 finding on #86067: after
1437+
// releaseForPrompt() opens the prompt window, heldLock is undefined but
1438+
// activePromptSessionTurn is still set until reacquireAfterPrompt clears
1439+
// it. If teardown runs in that window (mid-stream throw, etc.), dispose
1440+
// must release+clear the prompt turn or the file-scoped queue tail
1441+
// never resolves and the next same-file controller waits forever.
1442+
resetEmbeddedAttemptSessionFilePromptGuardsForTest();
1443+
const sessionFile = await createTempSessionFile();
1444+
const acquireSessionWriteLock = vi.fn(async () => ({
1445+
release: vi.fn(async () => {}),
1446+
}));
1447+
const controllerA = await createEmbeddedAttemptSessionLockController({
1448+
acquireSessionWriteLock,
1449+
lockOptions: { ...lockOptions, sessionFile },
1450+
});
1451+
1452+
await controllerA.releaseForPrompt();
1453+
// Teardown without going through reacquireAfterPrompt — the case the
1454+
// outer-finally `releaseRetainedSessionLock?.()` covers when the prompt
1455+
// stream throws.
1456+
await controllerA.dispose();
1457+
// Idempotent: second dispose must be safe (the outer finally can run
1458+
// even after acquireForCleanup has handed off the lock).
1459+
await controllerA.dispose();
1460+
1461+
// A new controller on the same file must be able to open its own
1462+
// prompt window without hanging on A's vacated turn.
1463+
const controllerB = await createEmbeddedAttemptSessionLockController({
1464+
acquireSessionWriteLock,
1465+
lockOptions: { ...lockOptions, sessionFile },
1466+
});
1467+
await expect(
1468+
Promise.race([
1469+
controllerB.releaseForPrompt().then(() => "released"),
1470+
new Promise<string>((resolve) => setTimeout(() => resolve("timeout"), 200)),
1471+
]),
1472+
).resolves.toBe("released");
1473+
expect(controllerB.hasSessionTakeover()).toBe(false);
1474+
});
12081475
});

0 commit comments

Comments
 (0)