Skip to content

Commit cb5caa0

Browse files
committed
Fix session cleanup memory leaks and add regression tests
1 parent d07d225 commit cb5caa0

4 files changed

Lines changed: 295 additions & 14 deletions

File tree

src/common/state.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,17 @@ const snippetsBySession = new Map<string, Map<string, FileSnippet>>();
2929
const snippetCountersBySession = new Map<string, number>();
3030
const fileVersionsBySession = new Map<string, Map<string, number>>();
3131

32+
export function clearSessionState(sessionId: string): void {
33+
if (!sessionId) {
34+
return;
35+
}
36+
37+
fileStatesBySession.delete(sessionId);
38+
snippetsBySession.delete(sessionId);
39+
snippetCountersBySession.delete(sessionId);
40+
fileVersionsBySession.delete(sessionId);
41+
}
42+
3243
export function normalizeFilePath(filePath: string, platform: NodeJS.Platform = process.platform): string {
3344
const nativePath = normalizeNativeFilePath(filePath, platform);
3445
return platform === "win32" ? path.win32.normalize(nativePath) : path.normalize(nativePath);

src/session.ts

Lines changed: 64 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ import {
4444
type PermissionToolCall,
4545
type UserToolPermission,
4646
} from "./common/permissions";
47+
import { clearSessionWorkingDir } from "./tools/bash-handler";
48+
import { clearSessionState } from "./common/state";
4749

4850
export type { PermissionScope } from "./settings";
4951
export type {
@@ -346,6 +348,18 @@ export class SessionManager {
346348
}
347349

348350
dispose(): void {
351+
const controller = this.activePromptController;
352+
if (controller && !controller.signal.aborted) {
353+
controller.abort();
354+
}
355+
this.activePromptController = null;
356+
for (const sessionController of this.sessionControllers.values()) {
357+
if (!sessionController.signal.aborted) {
358+
sessionController.abort();
359+
}
360+
}
361+
this.sessionControllers.clear();
362+
this.processTimeoutControls.clear();
349363
this.mcpManager.disconnect();
350364
}
351365

@@ -979,7 +993,12 @@ The candidate skills are as follows:\n\n`;
979993
const droppedEntries = sortedEntries.filter((item) => !keptIds.has(item.id));
980994
index.entries = keptEntries;
981995
this.saveSessionsIndex(index);
982-
this.removeSessionMessages(droppedEntries.map((item) => item.id));
996+
for (const dropped of droppedEntries) {
997+
this.cleanupSessionResources(dropped.id, {
998+
removeMessages: true,
999+
processIds: this.getProcessIds(dropped.processes ?? null),
1000+
});
1001+
}
9831002

9841003
const promptToolOptions = this.getPromptToolOptions();
9851004
const systemPrompt = getSystemPrompt(this.projectRoot, promptToolOptions);
@@ -1588,25 +1607,20 @@ ${skillMd}
15881607
return index.entries.find((entry) => entry.id === sessionId) ?? null;
15891608
}
15901609

1591-
/**
1592-
* Delete a session by its ID.
1593-
* Removes the session entry from the index and deletes the associated messages file.
1594-
* Returns true if the session was found and deleted, false otherwise.
1595-
*/
15961610
deleteSession(sessionId: string): boolean {
15971611
const index = this.loadSessionsIndex();
1598-
const entryIndex = index.entries.findIndex((entry) => entry.id === sessionId);
1599-
if (entryIndex === -1) {
1612+
const targetEntry = index.entries.find((entry) => entry.id === sessionId) ?? null;
1613+
const nextEntries = index.entries.filter((entry) => entry.id !== sessionId);
1614+
if (nextEntries.length === index.entries.length) {
16001615
return false;
16011616
}
16021617

1603-
// Remove from index
1604-
index.entries.splice(entryIndex, 1);
1618+
index.entries = nextEntries;
16051619
this.saveSessionsIndex(index);
1606-
1607-
// Remove messages file
1608-
this.removeSessionMessages([sessionId]);
1609-
1620+
this.cleanupSessionResources(sessionId, {
1621+
removeMessages: true,
1622+
processIds: this.getProcessIds(targetEntry?.processes ?? null),
1623+
});
16101624
return true;
16111625
}
16121626

@@ -1853,6 +1867,42 @@ ${skillMd}
18531867
}
18541868
}
18551869

1870+
private cleanupSessionResources(
1871+
sessionId: string,
1872+
options: { removeMessages: boolean; processIds?: number[] }
1873+
): void {
1874+
const processIds = options.processIds ?? [];
1875+
for (const pid of processIds) {
1876+
const processControlKey = this.getProcessControlKey(sessionId, pid);
1877+
if (!this.processTimeoutControls.has(processControlKey)) {
1878+
continue;
1879+
}
1880+
1881+
const killedGroup = killProcessTree(pid, "SIGKILL");
1882+
if (killedGroup) {
1883+
this.processTimeoutControls.delete(processControlKey);
1884+
continue;
1885+
}
1886+
try {
1887+
process.kill(pid, "SIGKILL");
1888+
} catch {
1889+
// ignore process-kill failures during cleanup
1890+
}
1891+
this.processTimeoutControls.delete(processControlKey);
1892+
}
1893+
1894+
clearSessionState(sessionId);
1895+
clearSessionWorkingDir(sessionId);
1896+
const controller = this.sessionControllers.get(sessionId);
1897+
if (controller && !controller.signal.aborted) {
1898+
controller.abort();
1899+
}
1900+
this.sessionControllers.delete(sessionId);
1901+
if (options.removeMessages) {
1902+
this.removeSessionMessages([sessionId]);
1903+
}
1904+
}
1905+
18561906
private appendSessionMessage(sessionId: string, message: SessionMessage): void {
18571907
this.ensureProjectDir();
18581908
const messagePath = this.getSessionMessagesPath(sessionId);

src/tests/memory-leak.test.ts

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,213 @@
1+
import { afterEach, test } from "node:test";
2+
import assert from "node:assert/strict";
3+
import * as fs from "fs";
4+
import * as os from "os";
5+
import * as path from "path";
6+
import { SessionManager } from "../session";
7+
import { handleBashTool } from "../tools/bash-handler";
8+
import * as state from "../common/state";
9+
import type { ToolExecutionContext } from "../tools/executor";
10+
11+
const originalHome = process.env.HOME;
12+
const originalUserProfile = process.env.USERPROFILE;
13+
const tempDirs: string[] = [];
14+
15+
function createTempDir(prefix: string): string {
16+
const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix));
17+
tempDirs.push(dir);
18+
return dir;
19+
}
20+
21+
function setHomeDir(dir: string): void {
22+
process.env.HOME = dir;
23+
if (process.platform === "win32") {
24+
process.env.USERPROFILE = dir;
25+
}
26+
}
27+
28+
function createSessionManager(projectRoot: string): SessionManager {
29+
return new SessionManager({
30+
projectRoot,
31+
createOpenAIClient: () => ({
32+
client: null,
33+
model: "test",
34+
baseURL: "https://api.test.com",
35+
thinkingEnabled: false,
36+
reasoningEffort: "high",
37+
debugLogEnabled: false,
38+
env: {},
39+
}),
40+
getResolvedSettings: () => ({ model: "test" }),
41+
renderMarkdown: (text: string) => text,
42+
onAssistantMessage: () => {},
43+
});
44+
}
45+
46+
afterEach(() => {
47+
if (originalHome === undefined) {
48+
delete process.env.HOME;
49+
} else {
50+
process.env.HOME = originalHome;
51+
}
52+
if (originalUserProfile === undefined) {
53+
delete process.env.USERPROFILE;
54+
} else {
55+
process.env.USERPROFILE = originalUserProfile;
56+
}
57+
58+
while (tempDirs.length > 0) {
59+
const dir = tempDirs.pop();
60+
if (dir) {
61+
fs.rmSync(dir, { recursive: true, force: true });
62+
}
63+
}
64+
});
65+
66+
test("SessionManager.deleteSession clears state cache for that session", async () => {
67+
const home = createTempDir("deepcode-mem-home-");
68+
const projectRoot = createTempDir("deepcode-mem-workspace-");
69+
setHomeDir(home);
70+
const manager = createSessionManager(projectRoot);
71+
72+
const sessionId = await manager.createSession({ text: "seed" });
73+
const filePath = path.join(projectRoot, "a.txt");
74+
fs.writeFileSync(filePath, "hello");
75+
state.recordFileState(sessionId, { filePath, content: "hello", timestamp: Date.now() }, { incrementVersion: true });
76+
const snippet = state.createSnippet(sessionId, filePath, 1, 1, "hello");
77+
const fileVersionBeforeDelete = state.getFileVersion(sessionId, filePath);
78+
79+
assert.ok(state.wasFileRead(sessionId, filePath));
80+
assert.ok(snippet);
81+
assert.ok(state.getSnippet(sessionId, snippet!.id));
82+
assert.equal(fileVersionBeforeDelete, 1);
83+
84+
assert.equal(manager.deleteSession(sessionId), true);
85+
assert.equal(state.wasFileRead(sessionId, filePath), false);
86+
assert.equal(state.getSnippet(sessionId, snippet!.id), null);
87+
assert.equal(state.getFileVersion(sessionId, filePath), 0);
88+
});
89+
90+
test("SessionManager.createSession auto-prune clears dropped session state cache", async () => {
91+
const home = createTempDir("deepcode-mem-home-");
92+
const projectRoot = createTempDir("deepcode-mem-workspace-");
93+
setHomeDir(home);
94+
const manager = createSessionManager(projectRoot);
95+
96+
const firstSession = await manager.createSession({ text: "first" });
97+
const filePath = path.join(projectRoot, "first.txt");
98+
fs.writeFileSync(filePath, "first");
99+
state.recordFileState(firstSession, { filePath, content: "first", timestamp: Date.now() });
100+
assert.equal(state.wasFileRead(firstSession, filePath), true);
101+
102+
for (let i = 0; i < 60; i += 1) {
103+
await manager.createSession({ text: `session-${i}` });
104+
}
105+
106+
const remaining = manager.listSessions().map((entry) => entry.id);
107+
assert.equal(remaining.includes(firstSession), false);
108+
assert.equal(state.wasFileRead(firstSession, filePath), false);
109+
});
110+
111+
test("SessionManager.deleteSession clears controller map entry", async () => {
112+
const home = createTempDir("deepcode-mem-home-");
113+
const projectRoot = createTempDir("deepcode-mem-workspace-");
114+
setHomeDir(home);
115+
const manager = createSessionManager(projectRoot);
116+
117+
const sessionId = await manager.createSession({ text: "seed" });
118+
const controllers = (manager as unknown as { sessionControllers: Map<string, AbortController> }).sessionControllers;
119+
controllers.set(sessionId, new AbortController());
120+
assert.equal(controllers.has(sessionId), true);
121+
122+
assert.equal(manager.deleteSession(sessionId), true);
123+
assert.equal(controllers.has(sessionId), false);
124+
});
125+
126+
test("SessionManager.dispose aborts and clears controllers", () => {
127+
const projectRoot = createTempDir("deepcode-mem-workspace-");
128+
const manager = createSessionManager(projectRoot);
129+
const controllers = (manager as unknown as { sessionControllers: Map<string, AbortController> }).sessionControllers;
130+
131+
const controllerA = new AbortController();
132+
const controllerB = new AbortController();
133+
controllers.set("a", controllerA);
134+
controllers.set("b", controllerB);
135+
assert.equal(controllers.size, 2);
136+
137+
manager.dispose();
138+
assert.equal(controllers.size, 0);
139+
});
140+
141+
test("Deleted session id reuse should reset bash cwd to project root", async () => {
142+
const home = createTempDir("deepcode-mem-home-");
143+
const projectRoot = createTempDir("deepcode-mem-workspace-");
144+
setHomeDir(home);
145+
const manager = createSessionManager(projectRoot);
146+
147+
const sessionId = await manager.createSession({ text: "bash-session" });
148+
const sub = path.join(projectRoot, "sub");
149+
fs.mkdirSync(sub, { recursive: true });
150+
151+
const context: ToolExecutionContext = {
152+
sessionId,
153+
projectRoot,
154+
toolCall: { id: "call-1", type: "function", function: { name: "bash", arguments: "{}" } },
155+
createOpenAIClient: () => ({
156+
client: null,
157+
model: "test",
158+
baseURL: "",
159+
thinkingEnabled: false,
160+
reasoningEffort: "high",
161+
debugLogEnabled: false,
162+
env: {},
163+
}),
164+
};
165+
166+
const first = await handleBashTool({ command: `cd "${sub}" && pwd` }, context);
167+
assert.equal(first.ok, true);
168+
169+
assert.equal(manager.deleteSession(sessionId), true);
170+
171+
const second = await handleBashTool({ command: "pwd" }, context);
172+
assert.equal(second.ok, true);
173+
174+
const output = (second.output ?? "").trim();
175+
const normalizedRoot = fs.realpathSync(projectRoot);
176+
assert.ok(output.startsWith(normalizedRoot), `expected cwd to reset to ${normalizedRoot}, got ${output}`);
177+
});
178+
179+
test("deleteSession should not kill untracked stale persisted pids", async () => {
180+
const home = createTempDir("deepcode-mem-home-");
181+
const projectRoot = createTempDir("deepcode-mem-workspace-");
182+
setHomeDir(home);
183+
const manager = createSessionManager(projectRoot);
184+
const sessionId = await manager.createSession({ text: "stale-pid" });
185+
186+
const privateManager = manager as unknown as {
187+
updateSessionEntry: (
188+
sessionId: string,
189+
updater: (entry: { processes: Map<string, { startTime: string; command: string }> | null }) => {
190+
processes: Map<string, { startTime: string; command: string }> | null;
191+
}
192+
) => unknown;
193+
};
194+
privateManager.updateSessionEntry(sessionId, (entry) => ({
195+
...entry,
196+
processes: new Map([["999999", { startTime: new Date().toISOString(), command: "sleep 999" }]]),
197+
}));
198+
199+
const originalKill = process.kill;
200+
let killCalls = 0;
201+
const mockedKill = ((pid: number, signal?: NodeJS.Signals | number) => {
202+
killCalls += 1;
203+
return originalKill(pid, signal);
204+
}) as typeof process.kill;
205+
process.kill = mockedKill;
206+
try {
207+
assert.equal(manager.deleteSession(sessionId), true);
208+
} finally {
209+
process.kill = originalKill;
210+
}
211+
212+
assert.equal(killCalls, 0);
213+
});

src/tools/bash-handler.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,13 @@ const MAX_OUTPUT_CHARS = 30000;
1515
const MAX_CAPTURE_CHARS = 10 * 1024 * 1024;
1616
const sessionWorkingDirs = new Map<string, string>();
1717

18+
export function clearSessionWorkingDir(sessionId: string): void {
19+
if (!sessionId) {
20+
return;
21+
}
22+
sessionWorkingDirs.delete(sessionId);
23+
}
24+
1825
type ToolCommandResult = {
1926
ok: boolean;
2027
output: string;

0 commit comments

Comments
 (0)