fix: add cleanup mechanisms to prevent memory leaks #7039
Conversation
… and OAuth - ACPSessionManager: add remove(), size(), and clear() methods for session cleanup - Rpc: add timeout mechanism (30s default) for pending requests to prevent orphaned calls - Rpc: add dispose() and pendingCount() methods for cleanup and monitoring - FileTime: add clearSession() and sessionCount() for per-session read time cleanup - MCP: add TTL (10 minutes) for pending OAuth transports to clean up abandoned flows Related to anomalyco#4315, anomalyco#5363
|
The following comment was made by an LLM, it may be inaccurate: Duplicate PR Search Results✓ Found 1 Related PR: PR #7032: fix(core): add dispose functions to prevent subscription memory leaks
SummaryNo duplicates found. PR #7039 is a new PR addressing memory leaks through cleanup mechanisms in ACPSessionManager, RPC, FileTime, and MCP OAuth. The only related open PR is #7032, which the PR description already acknowledges as complementary work - they target different areas of memory accumulation and are meant to work together. The PR is ready to proceed and does not appear to be a duplicate of any existing work. |
There was a problem hiding this comment.
Pull request overview
This PR adds cleanup mechanisms to prevent memory leaks across multiple components of the OpenCode system. The changes introduce TTL-based cleanup for OAuth flows, timeout handling for RPC calls, and management methods for session state cleanup.
Key changes:
- Added timeout mechanism (30s default) for RPC calls to prevent orphaned requests
- Implemented TTL-based cleanup (10 minutes) for pending OAuth transports
- Added session management methods (
remove(),size(),clear()) to ACPSessionManager and FileTime
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/util/rpc.ts |
Adds timeout mechanism, dispose(), and pendingCount() methods to prevent pending request accumulation |
src/mcp/index.ts |
Adds TTL-based cleanup for pendingOAuthTransports to handle abandoned authentication flows |
src/file/time.ts |
Adds clearSession() and sessionCount() methods for per-session read timestamp cleanup |
src/acp/session.ts |
Adds remove(), size(), and clear() methods for session state management |
test/util/rpc.test.ts |
Comprehensive tests for RPC timeout, disposal, and pending request tracking |
test/file/time.test.ts |
Tests for FileTime session cleanup functionality |
test/acp/session.test.ts |
Tests for ACPSessionManager cleanup methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) |
There was a problem hiding this comment.
There's a potential race condition between the timeout callback (lines 52-58) and the response handler (lines 36-46). If a response arrives at exactly the same time as the timeout fires, both could execute simultaneously. While the current implementation uses Map.get() and Map.delete() which should handle this, consider checking if the request still exists after clearTimeout to make the race handling more explicit.
| /** | ||
| * Remove a session from the manager to free memory. | ||
| * Should be called when a session is terminated or the connection closes. | ||
| */ | ||
| remove(sessionId: string): boolean { | ||
| const existed = this.sessions.has(sessionId) | ||
| if (existed) { | ||
| this.sessions.delete(sessionId) | ||
| log.info("removed_session", { sessionId }) | ||
| } | ||
| return existed | ||
| } | ||
|
|
||
| /** | ||
| * Get the count of active sessions (useful for monitoring/debugging). | ||
| */ | ||
| size(): number { | ||
| return this.sessions.size | ||
| } | ||
|
|
||
| /** | ||
| * Clear all sessions. Used during cleanup/dispose. | ||
| */ | ||
| clear(): void { | ||
| const count = this.sessions.size | ||
| this.sessions.clear() | ||
| log.info("cleared_all_sessions", { count }) | ||
| } |
There was a problem hiding this comment.
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.
| /** | ||
| * 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 | ||
| } |
There was a problem hiding this comment.
The FileTime.clearSession() method is not being called anywhere in the production code. While the method provides the infrastructure for memory management, there are no integration points where it would actually be invoked when a session ends. Consider adding calls to FileTime.clearSession() in session cleanup handlers or when sessions are removed.
| /** 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) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The timeout cleanup only runs when a timeout occurs, but expired transports can accumulate if OAuth flows are abandoned before timeout. Consider implementing periodic cleanup or cleanup on every insertion to handle abandoned flows more proactively. For example, running cleanup on a timer or on every state initialization would prevent accumulation of entries that never timeout.
| try { | ||
| // Call finishAuth on the transport | ||
| await transport.finishAuth(authorizationCode) | ||
| await entry.transport.finishAuth(authorizationCode) |
There was a problem hiding this comment.
After calling finishAuth on the transport (line 822), the entry is deleted from pendingOAuthTransports at line 840 before the subsequent add() operation. If the add() call or any other operation fails after line 840, the transport is already removed and cannot be retried. Consider moving the deletion to after the entire operation succeeds to allow retry on partial failures.
| }, | ||
| /** Clear all pending requests (for cleanup) */ | ||
| dispose(): void { | ||
| for (const [requestId, request] of pending) { |
There was a problem hiding this comment.
Unused variable requestId.
| for (const [requestId, request] of pending) { | |
| for (const request of pending.values()) { |
| @@ -0,0 +1,89 @@ | |||
| import { describe, expect, test, beforeEach, afterEach } from "bun:test" | |||
There was a problem hiding this comment.
Unused imports afterEach, beforeEach.
| import { describe, expect, test, beforeEach, afterEach } from "bun:test" | |
| import { describe, expect, test } from "bun:test" |
| const sdk2 = { | ||
| session: { create: async () => ({ data: { id: "session-2" } }) }, | ||
| } as any |
There was a problem hiding this comment.
Unused variable sdk2.
| const sdk2 = { | |
| session: { create: async () => ({ data: { id: "session-2" } }) }, | |
| } as any |
| @@ -0,0 +1,126 @@ | |||
| import { describe, expect, test, beforeEach, mock } from "bun:test" | |||
There was a problem hiding this comment.
Unused imports beforeEach, mock.
| import { describe, expect, test, beforeEach, mock } from "bun:test" | |
| import { describe, expect, test } from "bun:test" |
1 participant
Summary
This PR adds cleanup mechanisms to prevent memory leaks in several areas of the codebase:
remove(),size(), andclear()methods to properly clean up session state when sessions endclearSession()to clean up per-session file read timestamps when sessions endChanges
src/acp/session.tsremove(),size(),clear()methodssrc/util/rpc.tsdispose(),pendingCount()methodssrc/file/time.tsclearSession(),sessionCount()methodssrc/mcp/index.tspendingOAuthTransportstest/acp/session.test.tstest/util/rpc.test.tstest/file/time.test.tsTesting
Related Issues
Note
This PR is complementary to #7032 which addresses Bus subscription cleanup. Together they should significantly reduce memory accumulation during long-running sessions.