-
Notifications
You must be signed in to change notification settings - Fork 6.2k
fix: add cleanup mechanisms to prevent memory leaks #7039
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,27 @@ export namespace FileTime { | |
| return state().read[sessionID]?.[file] | ||
| } | ||
|
|
||
| /** | ||
| * Clear all read timestamps for a session to free memory. | ||
| * Should be called when a session ends. | ||
| */ | ||
| export function clearSession(sessionID: string): boolean { | ||
| const s = state() | ||
| if (sessionID in s.read) { | ||
| delete s.read[sessionID] | ||
| log.info("cleared session read times", { sessionID }) | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
Comment on lines
+34
to
+46
|
||
|
|
||
| /** | ||
| * Get the count of tracked sessions (useful for monitoring/debugging). | ||
| */ | ||
| export function sessionCount(): number { | ||
| return Object.keys(state().read).length | ||
| } | ||
|
|
||
| export async function withLock<T>(filepath: string, fn: () => Promise<T>): Promise<T> { | ||
| const current = state() | ||
| const currentLock = current.locks.get(filepath) ?? Promise.resolve() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,8 +141,27 @@ export namespace MCP { | |
| } | ||
|
|
||
| // Store transports for OAuth servers to allow finishing auth | ||
| // Each entry includes a timestamp for TTL-based cleanup | ||
| type TransportWithAuth = StreamableHTTPClientTransport | SSEClientTransport | ||
| const pendingOAuthTransports = new Map<string, TransportWithAuth>() | ||
| type PendingOAuthEntry = { | ||
| transport: TransportWithAuth | ||
| createdAt: number | ||
| } | ||
| const pendingOAuthTransports = new Map<string, PendingOAuthEntry>() | ||
|
|
||
| /** TTL for pending OAuth transports (10 minutes) */ | ||
| const OAUTH_TRANSPORT_TTL = 10 * 60 * 1000 | ||
|
|
||
| /** Clean up expired OAuth transports */ | ||
| function cleanupExpiredOAuthTransports() { | ||
| const now = Date.now() | ||
| for (const [key, entry] of pendingOAuthTransports) { | ||
| if (now - entry.createdAt > OAUTH_TRANSPORT_TTL) { | ||
| log.info("cleaning up expired oauth transport", { key, age: now - entry.createdAt }) | ||
| pendingOAuthTransports.delete(key) | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+155
to
+164
|
||
|
|
||
| // Prompt cache types | ||
| type PromptInfo = Awaited<ReturnType<MCPClient["listPrompts"]>>["prompts"][number] | ||
|
|
@@ -364,7 +383,8 @@ export namespace MCP { | |
| }).catch((e) => log.debug("failed to show toast", { error: e })) | ||
| } else { | ||
| // Store transport for later finishAuth call | ||
| pendingOAuthTransports.set(key, transport) | ||
| cleanupExpiredOAuthTransports() | ||
| pendingOAuthTransports.set(key, { transport, createdAt: Date.now() }) | ||
| status = { status: "needs_auth" as const } | ||
| // Show toast for needs_auth | ||
| Bus.publish(TuiEvent.ToastShow, { | ||
|
|
@@ -739,7 +759,8 @@ export namespace MCP { | |
| } catch (error) { | ||
| if (error instanceof UnauthorizedError && capturedUrl) { | ||
| // Store transport for finishAuth | ||
| pendingOAuthTransports.set(mcpName, transport) | ||
| cleanupExpiredOAuthTransports() | ||
| pendingOAuthTransports.set(mcpName, { transport, createdAt: Date.now() }) | ||
| return { authorizationUrl: capturedUrl.toString() } | ||
| } | ||
| throw error | ||
|
|
@@ -790,15 +811,15 @@ export namespace MCP { | |
| * Complete OAuth authentication with the authorization code. | ||
| */ | ||
| export async function finishAuth(mcpName: string, authorizationCode: string): Promise<Status> { | ||
| const transport = pendingOAuthTransports.get(mcpName) | ||
| const entry = pendingOAuthTransports.get(mcpName) | ||
|
|
||
| if (!transport) { | ||
| if (!entry) { | ||
| throw new Error(`No pending OAuth flow for MCP server: ${mcpName}`) | ||
| } | ||
|
|
||
| try { | ||
| // Call finishAuth on the transport | ||
| await transport.finishAuth(authorizationCode) | ||
| await entry.transport.finishAuth(authorizationCode) | ||
|
Comment on lines
820
to
+822
|
||
|
|
||
| // Clear the code verifier after successful auth | ||
| await McpAuth.clearCodeVerifier(mcpName) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,9 @@ export namespace Rpc { | |||||
| [method: string]: (input: any) => any | ||||||
| } | ||||||
|
|
||||||
| /** Default timeout for RPC calls in milliseconds (30 seconds) */ | ||||||
| const DEFAULT_TIMEOUT = 30_000 | ||||||
|
|
||||||
| export function listen(rpc: Definition) { | ||||||
| onmessage = async (evt) => { | ||||||
| const parsed = JSON.parse(evt.data) | ||||||
|
|
@@ -13,30 +16,67 @@ export namespace Rpc { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| export function client<T extends Definition>(target: { | ||||||
| postMessage: (data: string) => void | null | ||||||
| onmessage: ((this: Worker, ev: MessageEvent<any>) => any) | null | ||||||
| }) { | ||||||
| const pending = new Map<number, (result: any) => void>() | ||||||
| type PendingRequest = { | ||||||
| resolve: (result: any) => void | ||||||
| reject: (error: Error) => void | ||||||
| timeout: ReturnType<typeof setTimeout> | ||||||
| } | ||||||
|
|
||||||
| export function client<T extends Definition>( | ||||||
| target: { | ||||||
| postMessage: (data: string) => void | null | ||||||
| onmessage: ((this: Worker, ev: MessageEvent<any>) => any) | null | ||||||
| }, | ||||||
| options?: { timeout?: number }, | ||||||
| ) { | ||||||
| const pending = new Map<number, PendingRequest>() | ||||||
| const timeout = options?.timeout ?? DEFAULT_TIMEOUT | ||||||
| let id = 0 | ||||||
|
|
||||||
| target.onmessage = async (evt) => { | ||||||
| const parsed = JSON.parse(evt.data) | ||||||
| if (parsed.type === "rpc.result") { | ||||||
| const resolve = pending.get(parsed.id) | ||||||
| if (resolve) { | ||||||
| resolve(parsed.result) | ||||||
| const request = pending.get(parsed.id) | ||||||
| if (request) { | ||||||
| clearTimeout(request.timeout) | ||||||
| pending.delete(parsed.id) | ||||||
| request.resolve(parsed.result) | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return { | ||||||
| call<Method extends keyof T>(method: Method, input: Parameters<T[Method]>[0]): Promise<ReturnType<T[Method]>> { | ||||||
| const requestId = id++ | ||||||
| return new Promise((resolve) => { | ||||||
| pending.set(requestId, resolve) | ||||||
| return new Promise((resolve, reject) => { | ||||||
| const timeoutHandle = setTimeout(() => { | ||||||
| const request = pending.get(requestId) | ||||||
| if (request) { | ||||||
| pending.delete(requestId) | ||||||
| reject(new Error(`RPC call '${String(method)}' timed out after ${timeout}ms`)) | ||||||
| } | ||||||
| }, timeout) | ||||||
|
Comment on lines
+52
to
+58
|
||||||
|
|
||||||
| pending.set(requestId, { | ||||||
| resolve, | ||||||
| reject, | ||||||
| timeout: timeoutHandle, | ||||||
| }) | ||||||
| target.postMessage(JSON.stringify({ type: "rpc.request", method, input, id: requestId })) | ||||||
| }) | ||||||
| }, | ||||||
| /** Get count of pending requests (for testing/monitoring) */ | ||||||
| pendingCount(): number { | ||||||
| return pending.size | ||||||
| }, | ||||||
| /** Clear all pending requests (for cleanup) */ | ||||||
| dispose(): void { | ||||||
| for (const [requestId, request] of pending) { | ||||||
|
||||||
| for (const [requestId, request] of pending) { | |
| for (const request of pending.values()) { |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,89 @@ | ||||||||
| import { describe, expect, test, beforeEach, afterEach } from "bun:test" | ||||||||
|
||||||||
| import { describe, expect, test, beforeEach, afterEach } from "bun:test" | |
| import { describe, expect, test } from "bun:test" |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable sdk2.
| const sdk2 = { | |
| session: { create: async () => ({ data: { id: "session-2" } }) }, | |
| } as any |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| import { describe, expect, test } from "bun:test" | ||
| import { Instance } from "../../src/project/instance" | ||
| import { tmpdir } from "../fixture/fixture" | ||
|
|
||
| describe("FileTime", () => { | ||
| describe("clearSession", () => { | ||
| test("clears read times for a session and returns true", async () => { | ||
| await using tmp = await tmpdir({ git: true }) | ||
| await Instance.provide({ | ||
| directory: tmp.path, | ||
| fn: async () => { | ||
| const { FileTime } = await import("../../src/file/time") | ||
|
|
||
| // Record some reads | ||
| FileTime.read("session-1", "/path/to/file1.ts") | ||
| FileTime.read("session-1", "/path/to/file2.ts") | ||
| FileTime.read("session-2", "/path/to/file3.ts") | ||
|
|
||
| expect(FileTime.sessionCount()).toBe(2) | ||
| expect(FileTime.get("session-1", "/path/to/file1.ts")).toBeDefined() | ||
| expect(FileTime.get("session-1", "/path/to/file2.ts")).toBeDefined() | ||
| expect(FileTime.get("session-2", "/path/to/file3.ts")).toBeDefined() | ||
|
|
||
| // Clear session-1 | ||
| const result = FileTime.clearSession("session-1") | ||
| expect(result).toBe(true) | ||
|
|
||
| // Verify session-1 data is gone | ||
| expect(FileTime.get("session-1", "/path/to/file1.ts")).toBeUndefined() | ||
| expect(FileTime.get("session-1", "/path/to/file2.ts")).toBeUndefined() | ||
|
|
||
| // Verify session-2 data is still there | ||
| expect(FileTime.get("session-2", "/path/to/file3.ts")).toBeDefined() | ||
|
|
||
| expect(FileTime.sessionCount()).toBe(1) | ||
| }, | ||
| }) | ||
| }) | ||
|
|
||
| test("returns false for non-existent session", async () => { | ||
| await using tmp = await tmpdir({ git: true }) | ||
| await Instance.provide({ | ||
| directory: tmp.path, | ||
| fn: async () => { | ||
| const { FileTime } = await import("../../src/file/time") | ||
|
|
||
| const result = FileTime.clearSession("non-existent-session") | ||
| expect(result).toBe(false) | ||
| }, | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| describe("sessionCount", () => { | ||
| test("returns correct count of tracked sessions", async () => { | ||
| await using tmp = await tmpdir({ git: true }) | ||
| await Instance.provide({ | ||
| directory: tmp.path, | ||
| fn: async () => { | ||
| const { FileTime } = await import("../../src/file/time") | ||
|
|
||
| expect(FileTime.sessionCount()).toBe(0) | ||
|
|
||
| FileTime.read("session-a", "/file1.ts") | ||
| expect(FileTime.sessionCount()).toBe(1) | ||
|
|
||
| FileTime.read("session-b", "/file2.ts") | ||
| expect(FileTime.sessionCount()).toBe(2) | ||
|
|
||
| FileTime.read("session-a", "/file3.ts") // Same session | ||
| expect(FileTime.sessionCount()).toBe(2) | ||
|
|
||
| FileTime.clearSession("session-a") | ||
| expect(FileTime.sessionCount()).toBe(1) | ||
| }, | ||
| }) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new cleanup methods (remove, size, clear) are not being called anywhere in the production code. While they provide the infrastructure for memory management, the PR doesn't include integration points where these methods would actually be invoked (e.g., when an ACP connection closes or a session is terminated). Consider adding calls to sessionManager.remove() in the appropriate lifecycle hooks, such as the cancel method or connection cleanup handlers.