From c08455edad071fdd92a62fecc0f178e6ac7acaa5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Thu, 21 May 2026 12:34:38 +0700 Subject: [PATCH 1/2] fix(connections): cancelled connect no longer aborts a later successful connection (#1358) --- CHANGELOG.md | 4 + .../LibPQPluginConnection.swift | 2 +- .../Database/DatabaseManager+Sessions.swift | 42 +++++----- .../CancelledConnectionCleanupTests.swift | 80 +++++++++++++++++++ 4 files changed, 106 insertions(+), 22 deletions(-) create mode 100644 TableProTests/Core/Database/CancelledConnectionCleanupTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 930b10f0c..79e5771cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Canceling a pending connection no longer lets the abandoned attempt overwrite or drop a later successful connection to the same database (#1358) + ## [0.43.1] - 2026-05-20 ### Added diff --git a/Plugins/PostgreSQLDriverPlugin/LibPQPluginConnection.swift b/Plugins/PostgreSQLDriverPlugin/LibPQPluginConnection.swift index 8b102ef50..730a6baad 100644 --- a/Plugins/PostgreSQLDriverPlugin/LibPQPluginConnection.swift +++ b/Plugins/PostgreSQLDriverPlugin/LibPQPluginConnection.swift @@ -153,7 +153,7 @@ final class LibPQPluginConnection: @unchecked Sendable { // MARK: - Connection Management func connect() async throws { - try await pluginDispatchAsync(on: queue) { [self] in + try await pluginDispatchAsyncCancellable(on: queue) { [self] in func escapeConnParam(_ value: String) -> String { value.replacingOccurrences(of: "\\", with: "\\\\") .replacingOccurrences(of: "'", with: "\\'") diff --git a/TablePro/Core/Database/DatabaseManager+Sessions.swift b/TablePro/Core/Database/DatabaseManager+Sessions.swift index ce54d5b6c..1340aaef8 100644 --- a/TablePro/Core/Database/DatabaseManager+Sessions.swift +++ b/TablePro/Core/Database/DatabaseManager+Sessions.swift @@ -40,8 +40,7 @@ extension DatabaseManager { do { effectiveConnection = try await buildEffectiveConnection(for: resolvedConnection) } catch { - removeSessionEntry(for: connection.id) - currentSessionId = nil + finalizeConnectionFailure(for: connection.id, cancelled: Task.isCancelled) throw error } @@ -51,8 +50,7 @@ extension DatabaseManager { do { try await PreConnectHookRunner.run(script: script) } catch { - removeSessionEntry(for: connection.id) - currentSessionId = nil + finalizeConnectionFailure(for: connection.id, cancelled: Task.isCancelled) throw error } } @@ -68,8 +66,7 @@ extension DatabaseManager { isAPIToken: isApiOnly, window: NSApp.keyWindow ) else { - removeSessionEntry(for: connection.id) - currentSessionId = nil + finalizeConnectionFailure(for: connection.id, cancelled: Task.isCancelled) throw CancellationError() } passwordOverride = prompted @@ -84,7 +81,7 @@ extension DatabaseManager { awaitPlugins: true ) } catch { - if connection.resolvedSSHConfig.enabled { + if !Task.isCancelled, connection.resolvedSSHConfig.enabled { Task { do { try await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id) @@ -93,13 +90,13 @@ extension DatabaseManager { } } } - removeSessionEntry(for: connection.id) - currentSessionId = nil + finalizeConnectionFailure(for: connection.id, cancelled: Task.isCancelled) throw error } do { try await driver.connect() + try Task.checkCancellation() let timeoutSeconds = AppSettingsManager.shared.general.queryTimeoutSeconds do { @@ -122,6 +119,8 @@ extension DatabaseManager { for: connection, resolvedConnection: resolvedConnection, driver: driver ) + try Task.checkCancellation() + // Batch all session mutations into a single write to fire objectWillChange once. if var session = activeSessions[connection.id] { session.driver = driver @@ -144,7 +143,10 @@ extension DatabaseManager { await startHealthMonitor(for: connection.id) } } catch { - if connection.resolvedSSHConfig.enabled { + let cancelled = Task.isCancelled + if cancelled { + driver.disconnect() + } else if connection.resolvedSSHConfig.enabled { Task { do { try await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id) @@ -154,21 +156,19 @@ extension DatabaseManager { } } - // Remove failed session completely so UI returns to Welcome window. - removeSessionEntry(for: connection.id) - - if currentSessionId == connection.id { - if let nextSessionId = activeSessions.keys.first { - currentSessionId = nextSessionId - } else { - currentSessionId = nil - } - } - + finalizeConnectionFailure(for: connection.id, cancelled: cancelled) throw error } } + func finalizeConnectionFailure(for connectionId: UUID, cancelled: Bool) { + guard !cancelled else { return } + removeSessionEntry(for: connectionId) + if currentSessionId == connectionId { + currentSessionId = activeSessions.keys.first + } + } + private func executePostConnectActions( for connection: DatabaseConnection, resolvedConnection: DatabaseConnection, diff --git a/TableProTests/Core/Database/CancelledConnectionCleanupTests.swift b/TableProTests/Core/Database/CancelledConnectionCleanupTests.swift new file mode 100644 index 000000000..2fd70796e --- /dev/null +++ b/TableProTests/Core/Database/CancelledConnectionCleanupTests.swift @@ -0,0 +1,80 @@ +// +// CancelledConnectionCleanupTests.swift +// TableProTests +// +// Pins the fix for #1358: a cancelled connection attempt must not tear down the +// shared session entry, which after a cancel + retry belongs to the new attempt. +// + +import Foundation +@testable import TablePro +import TableProPluginKit +import Testing + +@Suite("Cancelled connection cleanup", .serialized) +@MainActor +struct CancelledConnectionCleanupTests { + @Test("Cancelled attempt leaves the session entry intact") + func cancelledLeavesSessionIntact() { + let id = UUID() + DatabaseManager.shared.injectSession( + ConnectionSession(connection: TestFixtures.makeConnection(id: id, name: "Retry")), + for: id + ) + defer { DatabaseManager.shared.removeSession(for: id) } + + DatabaseManager.shared.finalizeConnectionFailure(for: id, cancelled: true) + + #expect(DatabaseManager.shared.activeSessions[id] != nil) + } + + @Test("Genuine failure removes the session entry") + func genuineFailureRemovesSession() { + let id = UUID() + DatabaseManager.shared.injectSession( + ConnectionSession(connection: TestFixtures.makeConnection(id: id, name: "Failed")), + for: id + ) + defer { DatabaseManager.shared.removeSession(for: id) } + + DatabaseManager.shared.finalizeConnectionFailure(for: id, cancelled: false) + + #expect(DatabaseManager.shared.activeSessions[id] == nil) + } + + @Test("Cancelled finalize keeps currentSessionId untouched") + func cancelledKeepsCurrentSessionId() { + let id = UUID() + DatabaseManager.shared.injectSession( + ConnectionSession(connection: TestFixtures.makeConnection(id: id, name: "Retry")), + for: id + ) + DatabaseManager.shared.currentSessionId = id + defer { + DatabaseManager.shared.removeSession(for: id) + DatabaseManager.shared.currentSessionId = nil + } + + DatabaseManager.shared.finalizeConnectionFailure(for: id, cancelled: true) + + #expect(DatabaseManager.shared.currentSessionId == id) + } + + @Test("Genuine failure clears currentSessionId when no other session remains") + func genuineFailureClearsCurrentSessionId() { + let id = UUID() + DatabaseManager.shared.injectSession( + ConnectionSession(connection: TestFixtures.makeConnection(id: id, name: "Failed")), + for: id + ) + DatabaseManager.shared.currentSessionId = id + defer { + DatabaseManager.shared.removeSession(for: id) + DatabaseManager.shared.currentSessionId = nil + } + + DatabaseManager.shared.finalizeConnectionFailure(for: id, cancelled: false) + + #expect(DatabaseManager.shared.currentSessionId != id) + } +} From 9bdfc1b5f243d90cc8428ce63712b82f82f5d94f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Thu, 21 May 2026 12:55:43 +0700 Subject: [PATCH 2/2] refactor(connections): explicit ACL on finalizeConnectionFailure, fix changelog spelling, add session-switch test --- CHANGELOG.md | 2 +- .../Database/DatabaseManager+Sessions.swift | 2 +- .../CancelledConnectionCleanupTests.swift | 25 +++++++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 79e5771cc..03922ff04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Canceling a pending connection no longer lets the abandoned attempt overwrite or drop a later successful connection to the same database (#1358) +- Cancelling a pending connection no longer lets the abandoned attempt overwrite or drop a later successful connection to the same database (#1358) ## [0.43.1] - 2026-05-20 diff --git a/TablePro/Core/Database/DatabaseManager+Sessions.swift b/TablePro/Core/Database/DatabaseManager+Sessions.swift index 1340aaef8..3bb0ace24 100644 --- a/TablePro/Core/Database/DatabaseManager+Sessions.swift +++ b/TablePro/Core/Database/DatabaseManager+Sessions.swift @@ -161,7 +161,7 @@ extension DatabaseManager { } } - func finalizeConnectionFailure(for connectionId: UUID, cancelled: Bool) { + internal func finalizeConnectionFailure(for connectionId: UUID, cancelled: Bool) { guard !cancelled else { return } removeSessionEntry(for: connectionId) if currentSessionId == connectionId { diff --git a/TableProTests/Core/Database/CancelledConnectionCleanupTests.swift b/TableProTests/Core/Database/CancelledConnectionCleanupTests.swift index 2fd70796e..ee8d1f886 100644 --- a/TableProTests/Core/Database/CancelledConnectionCleanupTests.swift +++ b/TableProTests/Core/Database/CancelledConnectionCleanupTests.swift @@ -77,4 +77,29 @@ struct CancelledConnectionCleanupTests { #expect(DatabaseManager.shared.currentSessionId != id) } + + @Test("Genuine failure moves currentSessionId to a remaining session") + func genuineFailureSwitchesToRemainingSession() { + let failedId = UUID() + let otherId = UUID() + DatabaseManager.shared.injectSession( + ConnectionSession(connection: TestFixtures.makeConnection(id: failedId, name: "Failed")), + for: failedId + ) + DatabaseManager.shared.injectSession( + ConnectionSession(connection: TestFixtures.makeConnection(id: otherId, name: "Other")), + for: otherId + ) + DatabaseManager.shared.currentSessionId = failedId + defer { + DatabaseManager.shared.removeSession(for: failedId) + DatabaseManager.shared.removeSession(for: otherId) + DatabaseManager.shared.currentSessionId = nil + } + + DatabaseManager.shared.finalizeConnectionFailure(for: failedId, cancelled: false) + + #expect(DatabaseManager.shared.currentSessionId == otherId) + #expect(DatabaseManager.shared.activeSessions[otherId] != nil) + } }