Skip to content

Commit 0dbdaf9

Browse files
fuller-stack-devclawsweeper[bot]Takhoffman
authored
fix: release session lock before runtime teardown (#87747)
Summary: - The PR reorders embedded attempt cleanup to release the session write lock before session/MCP/LSP teardown, treats sessions_yield cleanup as abort-like for flush timing, and adds focused regression tests. - PR surface: Source +14, Tests +71. Total +85 across 3 files. - Reproducibility: yes. Source inspection shows current main releases the cleanup lock only after runtime tear ... R body’s terminal proof exercises the same ordering with production cleanup and filesystem lock primitives. Automerge notes: - PR branch already contained follow-up commit before automerge: Merge branch 'main' into fix/session-lock-release-before-teardown Validation: - ClawSweeper review passed for head 178192f. - Required merge gates passed before the squash merge. Prepared head SHA: 178192f Review: #87747 (comment) Co-authored-by: fuller-stack-dev <[email protected]> Co-authored-by: Jason (Json) <[email protected]> Co-authored-by: clawsweeper[bot] <274271284+clawsweeper[bot]@users.noreply.github.com> Approved-by: takhoffman Co-authored-by: takhoffman <[email protected]>
1 parent 59997d8 commit 0dbdaf9

3 files changed

Lines changed: 108 additions & 23 deletions

File tree

‎src/agents/embedded-agent-runner/run/attempt.subscription-cleanup.test.ts‎

Lines changed: 76 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ describe("cleanupEmbeddedAttemptResources", () => {
2121
vi.restoreAllMocks();
2222
});
2323

24-
it("waits for aborted prompt settlement before flushing, disposing, and releasing the lock", async () => {
24+
it("waits for aborted prompt settlement before flushing and releasing the lock", async () => {
2525
const order: string[] = [];
2626
const settle = createDeferred<void>();
2727

@@ -57,7 +57,7 @@ describe("cleanupEmbeddedAttemptResources", () => {
5757
settle.resolve();
5858
await cleanupPromise;
5959

60-
expect(order).toEqual(["guard", "flush", "dispose", "release"]);
60+
expect(order).toEqual(["guard", "flush", "release", "dispose"]);
6161
});
6262

6363
it("releases the lock after the aborted settle timeout", async () => {
@@ -93,7 +93,44 @@ describe("cleanupEmbeddedAttemptResources", () => {
9393
await vi.advanceTimersByTimeAsync(1);
9494
await cleanupPromise;
9595

96-
expect(order).toEqual(["flush", "dispose", "release"]);
96+
expect(order).toEqual(["flush", "release", "dispose"]);
97+
});
98+
99+
it("releases the lock before runtime teardown can hang", async () => {
100+
const order: string[] = [];
101+
let markRuntimeDisposeStarted!: () => void;
102+
const runtimeDisposeStarted = new Promise<void>((resolve) => {
103+
markRuntimeDisposeStarted = resolve;
104+
});
105+
106+
void cleanupEmbeddedAttemptResources({
107+
flushPendingToolResultsAfterIdle: vi.fn(async () => {
108+
order.push("flush");
109+
}),
110+
session: {
111+
agent: {},
112+
dispose: () => {
113+
order.push("dispose");
114+
},
115+
},
116+
sessionManager: {},
117+
sessionLock: {
118+
release: async () => {
119+
order.push("release");
120+
},
121+
},
122+
bundleMcpRuntime: {
123+
dispose: async () => {
124+
order.push("runtime-dispose-start");
125+
markRuntimeDisposeStarted();
126+
await new Promise(() => {});
127+
},
128+
},
129+
});
130+
131+
await runtimeDisposeStarted;
132+
133+
expect(order).toEqual(["flush", "release", "dispose", "runtime-dispose-start"]);
97134
});
98135

99136
it("does not wait for the settle promise on non-aborted cleanup", async () => {
@@ -116,10 +153,43 @@ describe("cleanupEmbeddedAttemptResources", () => {
116153
expect(release).toHaveBeenCalledTimes(1);
117154
});
118155

156+
it("still disposes resources when lock release fails", async () => {
157+
const releaseError = new Error("release failed");
158+
const dispose = vi.fn();
159+
const runtimeDispose = vi.fn(async () => {});
160+
161+
await expect(
162+
cleanupEmbeddedAttemptResources({
163+
flushPendingToolResultsAfterIdle: vi.fn(async () => {}),
164+
session: {
165+
agent: {},
166+
dispose,
167+
},
168+
sessionManager: {},
169+
sessionLock: {
170+
release: async () => {
171+
throw releaseError;
172+
},
173+
},
174+
bundleMcpRuntime: {
175+
dispose: runtimeDispose,
176+
},
177+
}),
178+
).rejects.toBe(releaseError);
179+
180+
expect(dispose).toHaveBeenCalledTimes(1);
181+
expect(runtimeDispose).toHaveBeenCalledTimes(1);
182+
});
183+
119184
it("can skip stale session-manager flushing after session takeover", async () => {
120185
const flushPendingToolResultsAfterIdle = vi.fn(async () => {});
121-
const dispose = vi.fn();
122-
const release = vi.fn(async () => {});
186+
const order: string[] = [];
187+
const dispose = vi.fn(() => {
188+
order.push("dispose");
189+
});
190+
const release = vi.fn(async () => {
191+
order.push("release");
192+
});
123193

124194
await cleanupEmbeddedAttemptResources({
125195
flushPendingToolResultsAfterIdle,
@@ -135,5 +205,6 @@ describe("cleanupEmbeddedAttemptResources", () => {
135205
expect(flushPendingToolResultsAfterIdle).not.toHaveBeenCalled();
136206
expect(dispose).toHaveBeenCalledTimes(1);
137207
expect(release).toHaveBeenCalledTimes(1);
208+
expect(order).toEqual(["release", "dispose"]);
138209
});
139210
});

‎src/agents/embedded-agent-runner/run/attempt.subscription-cleanup.ts‎

Lines changed: 25 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ export async function cleanupEmbeddedAttemptResources(params: {
7070
runId?: string;
7171
sessionId?: string;
7272
}): Promise<void> {
73+
let sessionLockReleaseError: unknown;
7374
try {
7475
try {
7576
params.removeToolResultContextGuard?.();
@@ -97,22 +98,31 @@ export async function cleanupEmbeddedAttemptResources(params: {
9798
/* best-effort */
9899
}
99100
}
101+
} finally {
100102
try {
101-
params.session?.dispose();
102-
} catch {
103-
/* best-effort */
104-
}
105-
try {
106-
await params.bundleMcpRuntime?.dispose();
107-
} catch {
108-
/* best-effort */
109-
}
110-
try {
111-
await params.bundleLspRuntime?.dispose();
112-
} catch {
113-
/* best-effort */
103+
await params.sessionLock.release();
104+
} catch (err) {
105+
sessionLockReleaseError = err;
114106
}
115-
} finally {
116-
await params.sessionLock.release();
107+
}
108+
109+
try {
110+
params.session?.dispose();
111+
} catch {
112+
/* best-effort */
113+
}
114+
try {
115+
await params.bundleMcpRuntime?.dispose();
116+
} catch {
117+
/* best-effort */
118+
}
119+
try {
120+
await params.bundleLspRuntime?.dispose();
121+
} catch {
122+
/* best-effort */
123+
}
124+
125+
if (sessionLockReleaseError) {
126+
throw sessionLockReleaseError;
117127
}
118128
}

‎src/agents/embedded-agent-runner/run/attempt.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1797,6 +1797,7 @@ export async function runEmbeddedAttempt(
17971797
let trajectoryRecorder: ReturnType<typeof createTrajectoryRuntimeRecorder> | null = null;
17981798
let trajectoryEndRecorded = false;
17991799
let buildAbortSettlePromise: () => Promise<void> | null = () => null;
1800+
let cleanupYieldAborted = false;
18001801
try {
18011802
await repairSessionFileIfNeeded({
18021803
sessionFile: params.sessionFile,
@@ -4011,6 +4012,7 @@ export async function runEmbeddedAttempt(
40114012
isRunnerAbortError(err) &&
40124013
err instanceof Error &&
40134014
err.cause === "sessions_yield";
4015+
cleanupYieldAborted = yieldAborted;
40144016
if (yieldAborted) {
40154017
aborted = false;
40164018
await waitForSessionsYieldAbortSettle({
@@ -4762,6 +4764,7 @@ export async function runEmbeddedAttempt(
47624764
timedOut ||
47634765
idleTimedOut ||
47644766
timedOutDuringCompaction;
4767+
const cleanupAbortLike = cleanupAborted || cleanupYieldAborted;
47654768
const cleanupSessionLock = await sessionLockController.acquireForCleanup({ session });
47664769
await cleanupEmbeddedAttemptResources({
47674770
removeToolResultContextGuard,
@@ -4771,9 +4774,10 @@ export async function runEmbeddedAttempt(
47714774
bundleMcpRuntime,
47724775
bundleLspRuntime,
47734776
sessionLock: cleanupSessionLock,
4774-
// PERF: If the run was aborted (user stop, timeout, etc.), skip the idle wait
4775-
// and flush pending results synchronously so we can release the session lock ASAP.
4776-
aborted: cleanupAborted,
4777+
// PERF: If the run was aborted (user stop, timeout, sessions_yield, etc.),
4778+
// skip the idle wait and flush pending results synchronously so we can
4779+
// release the session lock ASAP.
4780+
aborted: cleanupAbortLike,
47774781
abortSettlePromise: cleanupAborted ? buildAbortSettlePromise() : null,
47784782
skipSessionFlush: sessionLockController.hasSessionTakeover(),
47794783
runId: params.runId,

0 commit comments

Comments
 (0)