From f22cba833b0e60116f76738bbce95af0ed11f59e Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Mon, 9 Mar 2026 11:03:48 +0700 Subject: [PATCH 1/2] fix: clean up leaked observers, untracked tasks, and misleading log --- TablePro/Core/Database/DatabaseManager.swift | 17 +++++- .../MainContentCoordinator+URLFilter.swift | 11 ++-- .../Views/Main/MainContentCoordinator.swift | 7 ++- docs/development/resource-cleanup-audit.md | 61 +++++++++++++++++++ 4 files changed, 87 insertions(+), 9 deletions(-) create mode 100644 docs/development/resource-cleanup-audit.md diff --git a/TablePro/Core/Database/DatabaseManager.swift b/TablePro/Core/Database/DatabaseManager.swift index cff4cdee..1e8c0a17 100644 --- a/TablePro/Core/Database/DatabaseManager.swift +++ b/TablePro/Core/Database/DatabaseManager.swift @@ -34,6 +34,8 @@ final class DatabaseManager { /// Separate from the main driver so pings never queue behind long-running user queries. private var pingDrivers: [UUID: DatabaseDriver] = [:] + private var metadataCreationTasks: [UUID: Task] = [:] + /// Current session (computed from currentSessionId) var currentSession: ConnectionSession? { guard let sessionId = currentSessionId else { return nil } @@ -204,8 +206,9 @@ final class DatabaseManager { let metaConnection = effectiveConnection let metaConnectionId = connection.id let metaTimeout = AppSettingsManager.shared.general.queryTimeoutSeconds - Task { [weak self] in + metadataCreationTasks[metaConnectionId] = Task { [weak self] in guard let self else { return } + defer { self.metadataCreationTasks.removeValue(forKey: metaConnectionId) } do { let metaDriver = try DatabaseDriverFactory.createDriver(for: metaConnection) try await metaDriver.connect() @@ -269,6 +272,10 @@ final class DatabaseManager { try? await SSHTunnelManager.shared.closeTunnel(connectionId: session.connection.id) } + // Cancel any in-flight metadata driver creation + metadataCreationTasks[sessionId]?.cancel() + metadataCreationTasks.removeValue(forKey: sessionId) + // Stop health monitoring await stopHealthMonitor(for: sessionId) @@ -301,6 +308,9 @@ final class DatabaseManager { await stopHealthMonitor(for: sessionId) } + for task in metadataCreationTasks.values { task.cancel() } + metadataCreationTasks.removeAll() + let sessionIds = Array(activeSessions.keys) for sessionId in sessionIds { await disconnectSession(sessionId) @@ -539,7 +549,7 @@ final class DatabaseManager { } case .failed: Self.logger.error( - "Health monitoring failed for session \(id) after 3 retries") + "Health monitoring failed for session \(id)") self.updateSession(id) { session in session.status = .error(String(localized: "Connection lost")) session.clearCachedData() @@ -674,8 +684,9 @@ final class DatabaseManager { let metaTimeout = AppSettingsManager.shared.general.queryTimeoutSeconds let startupCmds = session.connection.startupCommands let connName = session.connection.name - Task { [weak self] in + metadataCreationTasks[metaConnectionId] = Task { [weak self] in guard let self else { return } + defer { self.metadataCreationTasks.removeValue(forKey: metaConnectionId) } do { let metaDriver = try DatabaseDriverFactory.createDriver(for: metaConnection) try await metaDriver.connect() diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+URLFilter.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+URLFilter.swift index 85606969..16f0bed7 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+URLFilter.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+URLFilter.swift @@ -6,9 +6,9 @@ import Foundation extension MainContentCoordinator { - func setupURLNotificationObservers() { + func setupURLNotificationObservers() -> [NSObjectProtocol] { let connId = connectionId - NotificationCenter.default.addObserver( + let observer1 = NotificationCenter.default.addObserver( forName: .applyURLFilter, object: nil, queue: .main @@ -17,7 +17,6 @@ extension MainContentCoordinator { let targetId = userInfo["connectionId"] as? UUID, targetId == connId else { return } - // Extract Sendable values before crossing isolation boundary let condition = userInfo["condition"] as? String let column = userInfo["column"] as? String let operation = userInfo["operation"] as? String @@ -30,7 +29,7 @@ extension MainContentCoordinator { } } - NotificationCenter.default.addObserver( + let observer2 = NotificationCenter.default.addObserver( forName: .switchSchemaFromURL, object: nil, queue: .main @@ -42,7 +41,7 @@ extension MainContentCoordinator { Task { @MainActor [weak self] in guard let self else { return } - + if self.connection.type == .postgresql { await self.switchSchema(to: schema) } else { @@ -50,6 +49,8 @@ extension MainContentCoordinator { } } } + + return [observer1, observer2] } private func applyURLFilterValues( diff --git a/TablePro/Views/Main/MainContentCoordinator.swift b/TablePro/Views/Main/MainContentCoordinator.swift index b5dc4c9e..8c9e2b89 100644 --- a/TablePro/Views/Main/MainContentCoordinator.swift +++ b/TablePro/Views/Main/MainContentCoordinator.swift @@ -77,6 +77,7 @@ final class MainContentCoordinator { @ObservationIgnored private var changeManagerUpdateTask: Task? @ObservationIgnored private var activeSortTasks: [UUID: Task] = [:] @ObservationIgnored private var terminationObserver: NSObjectProtocol? + @ObservationIgnored private var urlFilterObservers: [NSObjectProtocol] = [] /// Set during handleTabChange to suppress redundant onChange(of: resultColumns) reconfiguration @ObservationIgnored internal var isHandlingTabSwitch = false @@ -156,7 +157,7 @@ final class MainContentCoordinator { self.schemaProvider = SchemaProviderRegistry.shared.getOrCreate(for: connection.id) SchemaProviderRegistry.shared.retain(for: connection.id) - setupURLNotificationObservers() + urlFilterObservers = setupURLNotificationObservers() // Synchronous save at quit time. NotificationCenter with queue: .main // delivers the closure on the main thread, satisfying assumeIsolated's @@ -195,6 +196,10 @@ final class MainContentCoordinator { /// synchronously on MainActor so we don't depend on deinit + Task scheduling. func teardown() { _didTeardown.withLock { $0 = true } + for observer in urlFilterObservers { + NotificationCenter.default.removeObserver(observer) + } + urlFilterObservers.removeAll() if let observer = terminationObserver { NotificationCenter.default.removeObserver(observer) terminationObserver = nil diff --git a/docs/development/resource-cleanup-audit.md b/docs/development/resource-cleanup-audit.md new file mode 100644 index 00000000..b24cd9f1 --- /dev/null +++ b/docs/development/resource-cleanup-audit.md @@ -0,0 +1,61 @@ +# Resource Cleanup & Performance Audit + +Comprehensive audit of resource leaks, connection lifecycle edge cases, and performance issues related to [#214](https://github.com/datlechin/TablePro/issues/214). + +## Issues to Fix + +### 1. URL filter notification observers never removed +**Severity:** Minor | **File:** `Views/Main/Extensions/MainContentCoordinator+URLFilter.swift` + +`setupURLNotificationObservers()` (called from `init`) registers two block-based `NotificationCenter` observers but never stores the returned `NSObjectProtocol` tokens. They are never removed in `teardown()` or `deinit`. Each window open/close accumulates 2 orphaned observers. + +Uses `[weak self]` so no retain cycle, but dead observers still fire and filter by `connectionId` — wasted work. + +**Fix:** Store observer tokens and remove in `teardown()`. + +### 2. Untracked metadata driver creation Tasks +**Severity:** Important | **File:** `Core/Database/DatabaseManager.swift:207, 677` + +`connectToSession()` and `reconnectSession()` spawn fire-and-forget `Task {}` blocks to create metadata drivers. These tasks are never stored or cancelled. If `disconnectSession()` runs before the task completes, the task may assign a driver to a session that no longer exists (safe due to optional chaining) or — if a new session reuses the same UUID — to the wrong session. + +**Fix:** Store metadata creation tasks and cancel them in `disconnectSession()`. + +### 3. Misleading health monitor log message +**Severity:** Minor | **File:** `Core/Database/DatabaseManager.swift:542` + +Log says "after 3 retries" but `ConnectionHealthMonitor` retries indefinitely with exponential backoff (capped at 120s). The "3" is incorrect. + +**Fix:** Update log message. + +### 4. Database connections not cleaned up on app termination +**Severity:** Important | **File:** `AppDelegate.swift:806` + +`applicationWillTerminate` kills SSH tunnels via `terminateAllProcessesSync()` but does not disconnect database sessions. Health monitors, ping drivers, and database connections survive until the process exits. While the OS reclaims sockets, this skips graceful disconnect (e.g., PostgreSQL `PQfinish`, MySQL `mysql_close`). + +**Fix:** Call `DatabaseManager.shared.disconnectAllSync()` or iterate sessions synchronously. + +### 5. `localPortCandidates()` allocates 5001-element shuffled array +**Severity:** Minor | **File:** `Core/SSH/SSHTunnelManager.swift:249` + +`Array(60000...65000).shuffled()` creates a ~40KB array per tunnel creation. A random start with wrap-around achieves the same anti-collision goal at O(1). + +**Fix:** Use random offset iteration instead of full array shuffle. + +## Completed Fixes (PR #217) + +- [x] SSH tunnel not closed on window close — deterministic disconnect via `WindowLifecycleMonitor` + `.lastWindowDidClose` notification +- [x] SSH tunnel not closed on app termination — `terminateAllProcessesSync()` in `applicationWillTerminate` +- [x] `waitForProcessExit` hang on already-exited process — guard `isRunning`, timeout via `TaskGroup`, SIGKILL fallback +- [x] `closeAllTunnels()` not waiting for process exit — concurrent `TaskGroup` with timeout + +## No Action Needed (Verified Correct) + +- **NotificationCenter observers** in DatabaseManager, AppDelegate, WindowLifecycleMonitor, MainContentCoordinator (termination) — all properly stored and removed +- **NSEvent monitors** in VimKeyInterceptor, InlineSuggestionManager, SQLEditorCoordinator — dual cleanup (deinit + explicit uninstall) +- **Async Tasks** in MainContentCoordinator — all stored and cancelled in `teardown()` +- **ConnectionHealthMonitor** — `stopMonitoring()` cancels task and finishes continuation +- **Ping drivers** — disconnected in `stopHealthMonitor()` +- **SchemaProviderRegistry** — cleared in `disconnectSession()` +- **File handles** in ExportService, FileDecompressor — closed via `defer` blocks +- **Combine subscriptions** in SidebarViewModel — deallocated with ViewModel +- **URLSession instances** — all singletons with app lifetime, acceptable From 11567666c2c73d839572f86f73be7eb237d87070 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Mon, 9 Mar 2026 11:05:43 +0700 Subject: [PATCH 2/2] chore: remove resource cleanup audit doc from PR --- docs/development/resource-cleanup-audit.md | 61 ---------------------- 1 file changed, 61 deletions(-) delete mode 100644 docs/development/resource-cleanup-audit.md diff --git a/docs/development/resource-cleanup-audit.md b/docs/development/resource-cleanup-audit.md deleted file mode 100644 index b24cd9f1..00000000 --- a/docs/development/resource-cleanup-audit.md +++ /dev/null @@ -1,61 +0,0 @@ -# Resource Cleanup & Performance Audit - -Comprehensive audit of resource leaks, connection lifecycle edge cases, and performance issues related to [#214](https://github.com/datlechin/TablePro/issues/214). - -## Issues to Fix - -### 1. URL filter notification observers never removed -**Severity:** Minor | **File:** `Views/Main/Extensions/MainContentCoordinator+URLFilter.swift` - -`setupURLNotificationObservers()` (called from `init`) registers two block-based `NotificationCenter` observers but never stores the returned `NSObjectProtocol` tokens. They are never removed in `teardown()` or `deinit`. Each window open/close accumulates 2 orphaned observers. - -Uses `[weak self]` so no retain cycle, but dead observers still fire and filter by `connectionId` — wasted work. - -**Fix:** Store observer tokens and remove in `teardown()`. - -### 2. Untracked metadata driver creation Tasks -**Severity:** Important | **File:** `Core/Database/DatabaseManager.swift:207, 677` - -`connectToSession()` and `reconnectSession()` spawn fire-and-forget `Task {}` blocks to create metadata drivers. These tasks are never stored or cancelled. If `disconnectSession()` runs before the task completes, the task may assign a driver to a session that no longer exists (safe due to optional chaining) or — if a new session reuses the same UUID — to the wrong session. - -**Fix:** Store metadata creation tasks and cancel them in `disconnectSession()`. - -### 3. Misleading health monitor log message -**Severity:** Minor | **File:** `Core/Database/DatabaseManager.swift:542` - -Log says "after 3 retries" but `ConnectionHealthMonitor` retries indefinitely with exponential backoff (capped at 120s). The "3" is incorrect. - -**Fix:** Update log message. - -### 4. Database connections not cleaned up on app termination -**Severity:** Important | **File:** `AppDelegate.swift:806` - -`applicationWillTerminate` kills SSH tunnels via `terminateAllProcessesSync()` but does not disconnect database sessions. Health monitors, ping drivers, and database connections survive until the process exits. While the OS reclaims sockets, this skips graceful disconnect (e.g., PostgreSQL `PQfinish`, MySQL `mysql_close`). - -**Fix:** Call `DatabaseManager.shared.disconnectAllSync()` or iterate sessions synchronously. - -### 5. `localPortCandidates()` allocates 5001-element shuffled array -**Severity:** Minor | **File:** `Core/SSH/SSHTunnelManager.swift:249` - -`Array(60000...65000).shuffled()` creates a ~40KB array per tunnel creation. A random start with wrap-around achieves the same anti-collision goal at O(1). - -**Fix:** Use random offset iteration instead of full array shuffle. - -## Completed Fixes (PR #217) - -- [x] SSH tunnel not closed on window close — deterministic disconnect via `WindowLifecycleMonitor` + `.lastWindowDidClose` notification -- [x] SSH tunnel not closed on app termination — `terminateAllProcessesSync()` in `applicationWillTerminate` -- [x] `waitForProcessExit` hang on already-exited process — guard `isRunning`, timeout via `TaskGroup`, SIGKILL fallback -- [x] `closeAllTunnels()` not waiting for process exit — concurrent `TaskGroup` with timeout - -## No Action Needed (Verified Correct) - -- **NotificationCenter observers** in DatabaseManager, AppDelegate, WindowLifecycleMonitor, MainContentCoordinator (termination) — all properly stored and removed -- **NSEvent monitors** in VimKeyInterceptor, InlineSuggestionManager, SQLEditorCoordinator — dual cleanup (deinit + explicit uninstall) -- **Async Tasks** in MainContentCoordinator — all stored and cancelled in `teardown()` -- **ConnectionHealthMonitor** — `stopMonitoring()` cancels task and finishes continuation -- **Ping drivers** — disconnected in `stopHealthMonitor()` -- **SchemaProviderRegistry** — cleared in `disconnectSession()` -- **File handles** in ExportService, FileDecompressor — closed via `defer` blocks -- **Combine subscriptions** in SidebarViewModel — deallocated with ViewModel -- **URLSession instances** — all singletons with app lifetime, acceptable