From 4147f2efa51d02507a315743a54584f924cbcfb3 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Tue, 22 Sep 2026 22:27:58 +0700 Subject: [PATCH 1/3] fix(plugin-oracle): stop a health check closing a connection a statement is running on --- CHANGELOG.md | 2 + .../OracleConnectErrorClassifier.swift | 38 +++- .../OracleCoreConnection.swift | 198 +++++++++++++++--- .../TableProOracleCore/OracleCoreError.swift | 8 + .../OracleDisconnectReason.swift | 33 +++ .../OracleConnectErrorClassifierTests.swift | 45 ++++ .../OracleCoreErrorMessageTests.swift | 48 +++++ .../OraclePingDecisionTests.swift | 28 +++ .../OracleQueryGateTests.swift | 55 +++++ .../OraclePlugin+Diagnostics.swift | 11 + Plugins/OracleDriverPlugin/OraclePlugin.swift | 9 +- .../Database/DatabaseManager+Sessions.swift | 44 ++-- TablePro/Resources/Localizable.xcstrings | 18 ++ .../Autocomplete/SQLSchemaProviderTests.swift | 2 + .../SessionSwitchOperationTrackingTests.swift | 73 +++++++ .../Plugins/OracleConnectionErrorTests.swift | 5 + 16 files changed, 569 insertions(+), 48 deletions(-) create mode 100644 Packages/TableProOracle/Sources/TableProOracleCore/OracleDisconnectReason.swift create mode 100644 Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleCoreErrorMessageTests.swift create mode 100644 Packages/TableProOracle/Tests/TableProOracleCoreTests/OraclePingDecisionTests.swift create mode 100644 Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleQueryGateTests.swift create mode 100644 TableProTests/Core/Database/SessionSwitchOperationTrackingTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 17117d061e..47a1ef1ff2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Raw Oracle driver error in the schema switch failure dialog. (#3053) +- Oracle health check closing a connection a statement was still running on. (#3053) - Global saved query inside a folder missing from every other connection. (#3045) - Saved query and folder drawn nowhere when the folder holding it was gone. - Keyword accepted for a global saved query while another connection already held it. diff --git a/Packages/TableProOracle/Sources/TableProOracleCore/OracleConnectErrorClassifier.swift b/Packages/TableProOracle/Sources/TableProOracleCore/OracleConnectErrorClassifier.swift index 1580110622..188c3b5aef 100644 --- a/Packages/TableProOracle/Sources/TableProOracleCore/OracleConnectErrorClassifier.swift +++ b/Packages/TableProOracle/Sources/TableProOracleCore/OracleConnectErrorClassifier.swift @@ -48,15 +48,49 @@ public enum OracleConnectErrorClassifier { } } +/// Which OracleNIO failures leave the channel unusable. +/// +/// It mirrors OracleNIO's own `ConnectionStateMachine.shouldCloseConnection(reason:)`, which is +/// internal and so cannot be called. Disagreeing with it means the app keeps a channel OracleNIO +/// has already torn down, and the next statement on it fails for a reason nobody can act on. The +/// old three-code list did exactly that for a client-side close (#3053). +/// +/// `clientClosesConnection` and `clientClosedConnection` are the two OracleNIO refuses to classify +/// at all, because it raises them only from `OracleConnection.close()`: by the time one exists the +/// channel is gone, so they are unambiguously fatal here. public enum OracleChannelFatalCode { - public static func isChannelFatal(_ codeDescription: String) -> Bool { + public static func isChannelFatal(_ codeDescription: String, serverErrorNumber: Int? = nil) -> Bool { + if codeDescription.hasPrefix("unsupportedVerifierType") { + return true + } switch codeDescription { - case "connectionError", "messageDecodingFailure", "unexpectedBackendMessage": + case "clientClosesConnection", + "clientClosedConnection", + "failedToAddSSLHandler", + "failedToVerifyTLSCertificates", + "connectionError", + "messageDecodingFailure", + "missingParameter", + "unexpectedBackendMessage", + "serverVersionNotSupported", + "sidNotSupported", + "uncleanShutdown", + "unsupportedDataType", + "advancedNegotiationFailed", + "advancedNegotiationRequired", + "loginHandshakeTimedOut": return true + case "server": + return serverErrorNumber == 28 || serverErrorNumber == 600 default: return false } } + + /// Whether this side closed the channel, rather than the server or the protocol failing. + public static func isClientClose(_ codeDescription: String) -> Bool { + codeDescription == "clientClosesConnection" || codeDescription == "clientClosedConnection" + } } public enum OracleSSLClassifier { diff --git a/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreConnection.swift b/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreConnection.swift index 697ae2223a..ef1b336594 100644 --- a/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreConnection.swift +++ b/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreConnection.swift @@ -7,26 +7,65 @@ import OSLog private let osLogger = Logger(subsystem: "com.TablePro", category: "OracleCoreConnection") +/// Whether the caller now owns the channel, or how long somebody else has held it. +internal enum QueryGateTurn: Sendable, Equatable { + case acquired + case busy(for: Duration) +} + +/// What a health check should do with the turn it was given. +internal enum OraclePingDecision: Sendable, Equatable { + /// The probe owns the channel and may run, and may close it if it gets no answer. + case probe + /// Somebody else is using the channel, which answers the question better than a probe could. + case reportAlive + /// Nothing has moved on the channel for longer than any statement is allowed to run. + case reportWedged + + static func of(_ turn: QueryGateTurn, wedgedAfter: Duration) -> OraclePingDecision { + switch turn { + case .acquired: + return .probe + case .busy(let held): + return held > wedgedAfter ? .reportWedged : .reportAlive + } + } +} + /// OracleNIO does not support concurrent queries on a single connection. /// Sending a second statement while the first stream is active corrupts the /// state machine. This actor serializes all executeQuery calls. -private actor QueryGate { - private var busy = false +/// +/// Holding the gate is what gives a task the right to close the channel, so the gate also +/// records when the channel last went from idle to busy. A probe that cannot take a turn can +/// then tell "somebody is using this, which answers the question better than I could" from +/// "somebody has been stuck on this for longer than any statement is allowed to run". +internal actor QueryGate { + private var heldSince: ContinuousClock.Instant? private var waiters: [CheckedContinuation] = [] func acquire() async { - if !busy { - busy = true + if heldSince == nil { + heldSince = .now return } await withCheckedContinuation { waiters.append($0) } } + func takeTurnIfFree() -> QueryGateTurn { + guard let heldSince else { + self.heldSince = .now + return .acquired + } + return .busy(for: .now - heldSince) + } + func release() { if !waiters.isEmpty { + heldSince = .now waiters.removeFirst().resume() } else { - busy = false + heldSince = nil } } } @@ -128,6 +167,9 @@ public final class OracleCoreConnection: @unchecked Sendable { // own socket rather than leaking one per abandoned attempt. guard !attempt.withLock({ $0 }) else { try? await connection.close() + osLogger.notice( + "Closed the Oracle connection: \(OracleDisconnectReason.abandonedLoginAttempt.logDescription, privacy: .public)" + ) throw OracleCoreError.loginTimedOut } return connection @@ -235,10 +277,10 @@ public final class OracleCoreConnection: @unchecked Sendable { if let underlying = error.underlying { return String(describing: underlying) } - return error.description + return String(format: OracleCoreError.driverErrorFormat, error.code.description) } - public func disconnect() { + public func disconnect(reason: OracleDisconnectReason = .userRequested) { let connection = state.withLock { current -> OracleNIO.OracleConnection? in guard current.isConnected else { return nil } current.isConnected = false @@ -251,7 +293,7 @@ public final class OracleCoreConnection: @unchecked Sendable { Task { try? await connection.close() - osLogger.debug("Disconnected from Oracle") + osLogger.notice("Closed the Oracle connection: \(reason.logDescription, privacy: .public)") } } @@ -259,7 +301,7 @@ public final class OracleCoreConnection: @unchecked Sendable { /// way to abort an in-flight statement. The next query redials and restores /// the session schema, which is the same recovery a query timeout uses. public func cancelCurrentQuery() { - disconnect() + disconnect(reason: .queryCancelled) } public func applyQueryTimeout(_ seconds: Int) { @@ -270,15 +312,75 @@ public final class OracleCoreConnection: @unchecked Sendable { state.withLock { $0.sessionSchema = schema } } - /// A health check must never inherit the user's query timeout, which is - /// unlimited by default. Without its own deadline a dead socket can leave - /// the caller waiting forever instead of triggering a reconnect. + /// How long a statement may hold the channel before a probe that cannot get a turn treats it + /// as wedged rather than as evidence the connection is alive. It is the app's own + /// `max(queryTimeout, 300)` staleness rule, read from the timeout the app already told the + /// driver about, so the two cannot drift. An unlimited query timeout is the user saying a + /// statement may run for as long as it runs, and the floor still catches a socket that died + /// without closing. + private var wedgedStatementSeconds: Double { + max(Double(state.withLock { $0.queryTimeoutSeconds }), 300) + } + + /// Answers whether this connection still works, without ever taking the channel away from + /// whoever is using it. + /// + /// A probe may close a channel only while it owns it. The old shape wrapped `executeQuery` in + /// a ten second deadline, and `executeQuery` opens by waiting on the query gate, so the + /// deadline covered the queue rather than the round trip: a probe that never got a turn fired + /// `disconnect()` into a healthy statement, which OracleNIO reports to that statement as + /// `clientClosedConnection` (#3053). + /// + /// A statement already in flight answers the question better than a probe could, so a busy + /// channel reads as alive. Past ``wedgedStatementSeconds`` it reads as wedged instead, which + /// is what keeps the app's own stale-query escape valve working. + /// + /// It asks OracleNIO directly rather than running `SELECT 1` through the app's statement path: + /// there is no transaction role to admit, no autocommit flag to choose, and no silent redial, + /// so a connection that has gone away reports that it has gone away instead of reporting the + /// health of a replacement nobody asked for. public func ping() async throws { - _ = try await withOracleTimeout( - seconds: Self.pingTimeoutSeconds, - onTimeout: { [self] in disconnect() }, - operation: { [self] in try await executeQuery(OracleSchemaQueries.ping) } - ) + let turn = await queryGate.takeTurnIfFree() + switch OraclePingDecision.of(turn, wedgedAfter: .seconds(wedgedStatementSeconds)) { + case .reportAlive: + return + case .reportWedged: + osLogger.error( + "An Oracle statement has held the connection past the staleness limit; treating it as wedged" + ) + disconnect(reason: .wedgedStatement) + throw OracleCoreError.connectionClosed + case .probe: + break + } + + /// Read after the turn is taken, so the handle pinged is the one the channel holds now + /// rather than one a reconnect replaced while this was deciding. + guard let connection = state.withLock({ $0.isConnected ? $0.nioConnection : nil }) else { + await queryGate.release() + throw OracleCoreError.notConnected + } + guard !connection.isClosed else { + markConnectionDead(reason: .channelAlreadyClosed) + await queryGate.release() + throw OracleCoreError.connectionClosed + } + + do { + try await withOracleTimeout( + seconds: Self.pingTimeoutSeconds, + onTimeout: { [self] in disconnect(reason: .pingTimedOut) }, + operation: { try await connection.ping() } + ) + await queryGate.release() + } catch is OracleTimeoutError { + await queryGate.release() + throw OracleCoreError.connectionClosed + } catch { + let mapped = mapExecutionError(error) + await queryGate.release() + throw mapped + } } // MARK: - Query Execution @@ -296,7 +398,7 @@ public final class OracleCoreConnection: @unchecked Sendable { /// connection is marked dead, so a channel abandoned here would stay open on the server for the life /// of the process. Extracted in the same single `withLock` `disconnect()` uses, so two racing closers /// cannot both reach `close()`. - private func markConnectionDead() { + private func markConnectionDead(reason: OracleDisconnectReason) { let connection = state.withLock { current -> OracleNIO.OracleConnection? in current.isConnected = false let connection = current.nioConnection @@ -308,7 +410,9 @@ public final class OracleCoreConnection: @unchecked Sendable { Task { try? await connection.close() - osLogger.debug("Closed the Oracle connection after it was marked dead") + osLogger.notice( + "Closed the Oracle connection after it was marked dead: \(reason.logDescription, privacy: .public)" + ) } } @@ -472,18 +576,41 @@ public final class OracleCoreConnection: @unchecked Sendable { return try await withOracleTimeout( seconds: Double(timeoutSeconds), - onTimeout: { [self] in disconnect() }, + onTimeout: { [self] in disconnect(reason: .queryTimedOut) }, operation: operation ) } + /// `OracleSQLError.description` never reaches the user. OracleNIO's own documentation says these + /// errors "should not be forwareded to the end user, as they may leak sensitive information", + /// and forwarding one is how a closed channel came out as + /// `OracleSQLError(code: clientClosedConnection, ...)` in an alert (#3053). The server's own + /// message is the one worth showing; without it the code's name says more than its struct dump, + /// and the full description goes to the log instead. private func mapQueryError(_ sqlError: OracleSQLError) -> OracleCoreError { - guard OracleChannelFatalCode.isChannelFatal(sqlError.code.description) else { - return .queryFailed(sqlError.serverInfo?.message ?? sqlError.description) + let code = sqlError.code.description + guard OracleChannelFatalCode.isChannelFatal( + code, serverErrorNumber: sqlError.serverInfo.map { Int($0.number) } + ) else { + guard let serverMessage = sqlError.serverInfo?.message else { + osLogger.error("Oracle statement failed: \(String(describing: sqlError), privacy: .public)") + return .queryFailed(String(format: OracleCoreError.driverErrorFormat, code)) + } + return .queryFailed(serverMessage) } - markConnectionDead() - osLogger.error("Oracle connection reset after fatal protocol error: \(sqlError.code.description, privacy: .public)") - return .protocolError + + if OracleChannelFatalCode.isClientClose(code) { + markConnectionDead(reason: .channelAlreadyClosed) + osLogger.error("Oracle statement failed because this side had closed the channel: \(code, privacy: .public)") + return .connectionClosed + } + + markConnectionDead(reason: .fatalProtocolError) + osLogger.error("Oracle connection reset after a fatal error: \(code, privacy: .public)") + /// ORA-00028 and ORA-00600 end the session, and the server says why better than any + /// wording here could. Everything else that reaches this point is the protocol failing. + guard let serverMessage = sqlError.serverInfo?.message else { return .protocolError } + return .queryFailed(serverMessage) } /// A socket the system reclaimed while the app was suspended surfaces as a @@ -502,7 +629,7 @@ public final class OracleCoreConnection: @unchecked Sendable { case is CancellationError: return error default: - markConnectionDead() + markConnectionDead(reason: .transportError) let detail = String(describing: error) osLogger.error("Oracle connection reset after a transport error: \(detail, privacy: .public)") return OracleCoreError.queryFailed(detail) @@ -533,6 +660,25 @@ public final class OracleCoreConnection: @unchecked Sendable { } } + /// Runs a statement that only configures the session, retrying it once across a channel this + /// side closed. + /// + /// Replaying an arbitrary statement across a reconnect is never safe, because the new session + /// holds none of the old one's state. A session-setup statement is the exception by + /// construction: it is one of the statements ``reconnectedConnection()`` already replays for + /// itself on every reconnect, so running it again is what the connection would have done + /// anyway. The retry redials through that same path, and a transaction bound to the closed + /// session still fails at ``OracleSessionTransaction/admit(_:on:)`` rather than carrying on in + /// a session that holds none of its work. + public func executeSessionSetup(_ query: String) async throws -> OracleRawResult { + do { + return try await executeQuery(query) + } catch OracleCoreError.connectionClosed { + osLogger.notice("Retrying an Oracle session setup statement on a fresh connection") + return try await executeQuery(query) + } + } + private func collectRows( _ query: String, options: StatementOptions, diff --git a/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreError.swift b/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreError.swift index 715cfbb619..50395d189f 100644 --- a/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreError.swift +++ b/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreError.swift @@ -18,6 +18,7 @@ public enum OracleCoreError: LocalizedError, Sendable, Equatable { case connectionFailed(String) case queryFailed(String) case cancelled + case connectionClosed case protocolError case loginTimedOut case queryTimedOut @@ -41,6 +42,8 @@ public enum OracleCoreError: LocalizedError, Sendable, Equatable { return detail.isEmpty ? String(localized: "Query execution failed") : detail case .cancelled: return String(localized: "Query was cancelled") + case .connectionClosed: + return String(localized: "The Oracle connection closed while the statement was running. Run it again.") case .protocolError: return String(localized: "The server sent an unexpected message and the connection was reset. Run the query again.") case .loginTimedOut: @@ -81,6 +84,11 @@ public enum OracleCoreError: LocalizedError, Sendable, Equatable { } } + /// What a driver failure with no server message behind it says to the user. OracleNIO's own + /// error description is a struct dump it documents as unfit to show, so the code's name is + /// what carries across. + public static let driverErrorFormat = String(localized: "The Oracle driver reported an error (%@).") + /// The driver names the handshake step in its own vocabulary. These are the names /// a user can act on, and an unrecognized one falls back to the raw label rather /// than losing the only clue the dialog has. diff --git a/Packages/TableProOracle/Sources/TableProOracleCore/OracleDisconnectReason.swift b/Packages/TableProOracle/Sources/TableProOracleCore/OracleDisconnectReason.swift new file mode 100644 index 0000000000..da0f3ec24b --- /dev/null +++ b/Packages/TableProOracle/Sources/TableProOracleCore/OracleDisconnectReason.swift @@ -0,0 +1,33 @@ +import Foundation + +/// Why an Oracle channel was closed from this side. +/// +/// OracleNIO reports a client-side close to whatever statement was on the wire as +/// `clientClosedConnection`, and that error names neither the closer nor its reason. A report of +/// one is otherwise only as good as the reader's guess at which of these fired (#3053), so every +/// close says which it is and the log carries it. +public enum OracleDisconnectReason: Sendable, Equatable { + case userRequested + case queryCancelled + case queryTimedOut + case pingTimedOut + case wedgedStatement + case channelAlreadyClosed + case fatalProtocolError + case transportError + case abandonedLoginAttempt + + public var logDescription: String { + switch self { + case .userRequested: return "the app closed it" + case .queryCancelled: return "the query was cancelled" + case .queryTimedOut: return "the query timeout fired" + case .pingTimedOut: return "the health check got no answer" + case .wedgedStatement: return "a statement held it past the staleness limit" + case .channelAlreadyClosed: return "OracleNIO had already closed the channel" + case .fatalProtocolError: return "the server sent an unexpected message" + case .transportError: return "the transport failed" + case .abandonedLoginAttempt: return "the login attempt had already been given up on" + } + } +} diff --git a/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleConnectErrorClassifierTests.swift b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleConnectErrorClassifierTests.swift index edde27fff9..401a98ffaf 100644 --- a/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleConnectErrorClassifierTests.swift +++ b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleConnectErrorClassifierTests.swift @@ -90,6 +90,51 @@ final class OracleConnectErrorClassifierTests: XCTestCase { XCTAssertFalse(OracleChannelFatalCode.isChannelFatal("statementError")) } + /// The table mirrors OracleNIO's own `ConnectionStateMachine.shouldCloseConnection(reason:)`, + /// which is internal and so cannot be called. Every case it names is pinned here. + func testChannelFatalTableMirrorsOracleNIO() { + for code in [ + "clientClosesConnection", + "clientClosedConnection", + "failedToAddSSLHandler", + "failedToVerifyTLSCertificates", + "connectionError", + "messageDecodingFailure", + "missingParameter", + "unexpectedBackendMessage", + "serverVersionNotSupported", + "sidNotSupported", + "uncleanShutdown", + "unsupportedDataType", + "unsupportedVerifierType(0x939)", + "advancedNegotiationFailed", + "advancedNegotiationRequired", + "loginHandshakeTimedOut" + ] { + XCTAssertTrue(OracleChannelFatalCode.isChannelFatal(code), code) + } + + for code in ["statementCancelled", "nationalCharsetNotSupported", "missingStatement", "malformedStatement"] { + XCTAssertFalse(OracleChannelFatalCode.isChannelFatal(code), code) + } + } + + /// ORA-28 is the session being killed and ORA-600 an internal error; OracleNIO closes the + /// channel on both and on no other server error. + func testServerErrorsAreFatalOnlyForKilledSessions() { + XCTAssertTrue(OracleChannelFatalCode.isChannelFatal("server", serverErrorNumber: 28)) + XCTAssertTrue(OracleChannelFatalCode.isChannelFatal("server", serverErrorNumber: 600)) + XCTAssertFalse(OracleChannelFatalCode.isChannelFatal("server", serverErrorNumber: 942)) + XCTAssertFalse(OracleChannelFatalCode.isChannelFatal("server")) + } + + func testClientClosesAreToldApartFromProtocolFailures() { + XCTAssertTrue(OracleChannelFatalCode.isClientClose("clientClosedConnection")) + XCTAssertTrue(OracleChannelFatalCode.isClientClose("clientClosesConnection")) + XCTAssertFalse(OracleChannelFatalCode.isClientClose("connectionError")) + XCTAssertFalse(OracleChannelFatalCode.isClientClose("uncleanShutdown")) + } + func testTLSClassifierRecognizesOracleWalletAndCipherErrors() { XCTAssertEqual(OracleSSLClassifier.classifyTLSFailure("ORA-28759: failure to open file"), .clientCertRequired) XCTAssertEqual(OracleSSLClassifier.classifyTLSFailure("ORA-29024: Certificate validation failure"), .cipherMismatch) diff --git a/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleCoreErrorMessageTests.swift b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleCoreErrorMessageTests.swift new file mode 100644 index 0000000000..58173b917d --- /dev/null +++ b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleCoreErrorMessageTests.swift @@ -0,0 +1,48 @@ +@testable import TableProOracleCore +import XCTest + +/// OracleNIO documents its own errors as unfit to show: "These errors should not be forwareded to +/// the end user, as they may leak sensitive information." One reached an alert as +/// `OracleSQLError(code: clientClosedConnection, triggeredFromRequestInFile: ...)`. +final class OracleCoreErrorMessageTests: XCTestCase { + private let cases: [OracleCoreError] = [ + .notConnected, + .connectionFailed("listener refused the connection"), + .queryFailed("ORA-00942: table or view does not exist"), + .cancelled, + .connectionClosed, + .protocolError, + .loginTimedOut, + .queryTimedOut, + .transactionLost, + .authVerifierUnsupported(flag: "unsupportedVerifierType(0x939)"), + .authVersionNotSupported, + .authConnectionDropped(phase: "authentication"), + .loginHandshakeStalled(phase: "connect"), + .nativeEncryptionFailed(detail: "checksum mismatch"), + .nativeEncryptionRequired, + .tlsHandshakeFailed(kind: .cipherMismatch, serverMessage: "ORA-29024"), + .certificateUnavailable(field: .clientKey, path: "/tmp/key.pem") + ] + + func testNoMessageLeaksTheDriversOwnErrorStruct() { + for error in cases { + let message = error.errorDescription ?? "" + XCTAssertFalse(message.contains("OracleSQLError("), message) + XCTAssertFalse(message.contains("triggeredFromRequestInFile"), message) + XCTAssertFalse(message.isEmpty) + } + } + + func testAClosedChannelSaysSoAndSaysWhatToDo() { + let message = OracleCoreError.connectionClosed.errorDescription ?? "" + XCTAssertTrue(message.contains("closed"), message) + XCTAssertTrue(message.contains("again"), message) + } + + func testADriverErrorWithNoServerMessageNamesItsCode() { + let message = String(format: OracleCoreError.driverErrorFormat, "malformedStatement") + XCTAssertTrue(message.contains("malformedStatement"), message) + XCTAssertFalse(message.contains("OracleSQLError("), message) + } +} diff --git a/Packages/TableProOracle/Tests/TableProOracleCoreTests/OraclePingDecisionTests.swift b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OraclePingDecisionTests.swift new file mode 100644 index 0000000000..de6ec2e4df --- /dev/null +++ b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OraclePingDecisionTests.swift @@ -0,0 +1,28 @@ +@testable import TableProOracleCore +import XCTest + +/// #3053. A health check used to wrap the whole of `executeQuery` in a ten second deadline, and +/// `executeQuery` opens by waiting on the query gate, so the deadline covered the queue rather +/// than the round trip. A probe that never got a turn closed the channel anyway, and OracleNIO +/// reports that to the statement that was running as `clientClosedConnection`. +final class OraclePingDecisionTests: XCTestCase { + func testAProbeThatOwnsTheChannelMayRun() { + XCTAssertEqual(OraclePingDecision.of(.acquired, wedgedAfter: .seconds(300)), .probe) + } + + func testABusyChannelIsReportedAliveRatherThanClosed() { + XCTAssertEqual(OraclePingDecision.of(.busy(for: .seconds(11)), wedgedAfter: .seconds(300)), .reportAlive) + XCTAssertEqual(OraclePingDecision.of(.busy(for: .seconds(299)), wedgedAfter: .seconds(300)), .reportAlive) + } + + func testAChannelHeldPastTheStalenessLimitIsReportedWedged() { + XCTAssertEqual(OraclePingDecision.of(.busy(for: .seconds(301)), wedgedAfter: .seconds(300)), .reportWedged) + } + + /// The limit is the app's own `max(queryTimeout, 300)` rule, so a long query timeout moves it + /// and the escape valve still fires past it. + func testTheStalenessLimitFollowsTheQueryTimeout() { + XCTAssertEqual(OraclePingDecision.of(.busy(for: .seconds(400)), wedgedAfter: .seconds(600)), .reportAlive) + XCTAssertEqual(OraclePingDecision.of(.busy(for: .seconds(601)), wedgedAfter: .seconds(600)), .reportWedged) + } +} diff --git a/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleQueryGateTests.swift b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleQueryGateTests.swift new file mode 100644 index 0000000000..c6a6e1e333 --- /dev/null +++ b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleQueryGateTests.swift @@ -0,0 +1,55 @@ +@testable import TableProOracleCore +import XCTest + +/// The gate is what gives a task the right to close the channel, so a probe that cannot take a +/// turn must be told so rather than being allowed to act (#3053). +final class OracleQueryGateTests: XCTestCase { + func testATurnIsRefusedWhileTheChannelIsHeld() async { + let gate = QueryGate() + await gate.acquire() + + guard case .busy = await gate.takeTurnIfFree() else { + return XCTFail("A held gate must not hand out a second turn") + } + + await gate.release() + let afterRelease = await gate.takeTurnIfFree() + XCTAssertEqual(afterRelease, .acquired) + } + + func testAQueuedStatementKeepsTheChannelBusy() async { + let gate = QueryGate() + await gate.acquire() + + let queued = Task { await gate.acquire() } + try? await Task.sleep(for: .milliseconds(50)) + + guard case .busy = await gate.takeTurnIfFree() else { + return XCTFail("A gate with a waiter must not hand out a turn") + } + + await gate.release() + await queued.value + + guard case .busy = await gate.takeTurnIfFree() else { + return XCTFail("The waiter now holds the gate") + } + await gate.release() + } + + func testAHandoverRestartsTheHoldingClock() async { + let gate = QueryGate() + await gate.acquire() + let queued = Task { await gate.acquire() } + try? await Task.sleep(for: .milliseconds(120)) + + await gate.release() + await queued.value + + guard case .busy(let held) = await gate.takeTurnIfFree() else { + return XCTFail("The waiter now holds the gate") + } + XCTAssertLessThan(held, .milliseconds(100)) + await gate.release() + } +} diff --git a/Plugins/OracleDriverPlugin/OraclePlugin+Diagnostics.swift b/Plugins/OracleDriverPlugin/OraclePlugin+Diagnostics.swift index 402a81bf92..317418358a 100644 --- a/Plugins/OracleDriverPlugin/OraclePlugin+Diagnostics.swift +++ b/Plugins/OracleDriverPlugin/OraclePlugin+Diagnostics.swift @@ -50,6 +50,17 @@ extension OraclePlugin { ], supportURL: issuesURL ) + case .connectionClosed: + return PluginDiagnostic( + title: String(localized: "Connection Closed"), + message: message, + suggestedActions: [ + String(localized: "Run it again. TablePro opens a new connection to the server automatically."), + String(localized: "If this keeps happening, check for a VPN, firewall or connection manager between you and the server that drops sessions."), + String(localized: "Ask your DBA whether a resource profile or an idle-session limit is ending the session.") + ], + supportURL: issuesURL + ) case .protocolError: return PluginDiagnostic( title: String(localized: "Connection Reset"), diff --git a/Plugins/OracleDriverPlugin/OraclePlugin.swift b/Plugins/OracleDriverPlugin/OraclePlugin.swift index d9b733db28..803371a436 100644 --- a/Plugins/OracleDriverPlugin/OraclePlugin.swift +++ b/Plugins/OracleDriverPlugin/OraclePlugin.swift @@ -1083,9 +1083,14 @@ final class OraclePluginDriver: PluginDatabaseDriver, @unchecked Sendable { // MARK: - Schema Switching func switchSchema(to schema: String) async throws { - _ = try await rawQuery(OracleSchemaQueries.setCurrentSchema(schema)) + guard let core else { throw OraclePluginError(core: .notConnected) } + do { + _ = try await core.executeSessionSetup(OracleSchemaQueries.setCurrentSchema(schema)) + } catch let error as OracleCoreError { + throw error.asPluginError + } _currentSchema = schema - core?.noteSessionSchema(schema) + core.noteSessionSchema(schema) } /// Oracle has no real database concept; "switch database" is a schema switch. diff --git a/TablePro/Core/Database/DatabaseManager+Sessions.swift b/TablePro/Core/Database/DatabaseManager+Sessions.swift index 38c5affaec..85d79101d3 100644 --- a/TablePro/Core/Database/DatabaseManager+Sessions.swift +++ b/TablePro/Core/Database/DatabaseManager+Sessions.swift @@ -340,18 +340,20 @@ extension DatabaseManager { let grouping = pm?.schema.databaseGroupingStrategy ?? .byDatabase let sessionStartedAt = session(for: connectionId)?.connectedAt let adapter = try await sessionDriverGate.withExclusiveAccess(connectionId) { - try Task.checkCancellation() - guard session(for: connectionId)?.connectedAt == sessionStartedAt else { - throw CancellationError() - } - guard let adapter = self.driver(for: connectionId) as? PluginDriverAdapter else { - throw DatabaseError.notConnected - } - try await adapter.switchDatabase(to: database) - if grouping == .bySchema { - await resetSchema(on: adapter, to: pm?.schema.defaultSchemaName) + try await trackOperation(sessionId: connectionId) { + try Task.checkCancellation() + guard session(for: connectionId)?.connectedAt == sessionStartedAt else { + throw CancellationError() + } + guard let adapter = self.driver(for: connectionId) as? PluginDriverAdapter else { + throw DatabaseError.notConnected + } + try await adapter.switchDatabase(to: database) + if grouping == .bySchema { + await resetSchema(on: adapter, to: pm?.schema.defaultSchemaName) + } + return adapter } - return adapter } updateSession(connectionId) { session in session.browseDatabase = database @@ -457,15 +459,21 @@ extension DatabaseManager { throw DatabaseError.unsupportedOperation } + /// Counted as an operation, like every other turn on the session driver, so a scheduled + /// ping skips at its `queriesInFlight` guard instead of entering a driver that is not + /// thread-safe alongside this. Holding `sessionDriverGate` is not enough on its own: the + /// ping never asks for that gate. try await sessionDriverGate.withExclusiveAccess(connectionId) { - try Task.checkCancellation() - guard session(for: connectionId)?.connectedAt == sessionStartedAt else { - throw CancellationError() - } - guard let schemaDriver = driver(for: connectionId) as? SchemaSwitchable else { - throw DatabaseError.notConnected + try await trackOperation(sessionId: connectionId) { + try Task.checkCancellation() + guard session(for: connectionId)?.connectedAt == sessionStartedAt else { + throw CancellationError() + } + guard let schemaDriver = driver(for: connectionId) as? SchemaSwitchable else { + throw DatabaseError.notConnected + } + try await schemaDriver.switchSchema(to: schema) } - try await schemaDriver.switchSchema(to: schema) } updateSession(connectionId) { session in session.browseSchema = schema diff --git a/TablePro/Resources/Localizable.xcstrings b/TablePro/Resources/Localizable.xcstrings index 655f3d2eca..052e9b367a 100644 --- a/TablePro/Resources/Localizable.xcstrings +++ b/TablePro/Resources/Localizable.xcstrings @@ -182719,6 +182719,24 @@ }, "Clear Recents…" : { + }, + "Connection Closed" : { + + }, + "Run it again. TablePro opens a new connection to the server automatically." : { + + }, + "If this keeps happening, check for a VPN, firewall or connection manager between you and the server that drops sessions." : { + + }, + "Ask your DBA whether a resource profile or an idle-session limit is ending the session." : { + + }, + "The Oracle connection closed while the statement was running. Run it again." : { + + }, + "The Oracle driver reported an error (%@)." : { + } }, "version" : "1.1" diff --git a/TableProTests/Core/Autocomplete/SQLSchemaProviderTests.swift b/TableProTests/Core/Autocomplete/SQLSchemaProviderTests.swift index ba99bf508d..bf46b5c4c3 100644 --- a/TableProTests/Core/Autocomplete/SQLSchemaProviderTests.swift +++ b/TableProTests/Core/Autocomplete/SQLSchemaProviderTests.swift @@ -41,6 +41,7 @@ final class MockDatabaseDriver: DatabaseDriver, SchemaSwitchable, @unchecked Sen var pingDelaySeconds: Double = 0 var connectDelaySeconds: Double = 0 var switchSchemaDelaySeconds: Double = 0 + var onSwitchSchema: (@Sendable () async -> Void)? var executeDelaySeconds: Double = 0 var hangsUntilDisconnect = false var schemasToReturn: [String] = [] @@ -199,6 +200,7 @@ final class MockDatabaseDriver: DatabaseDriver, SchemaSwitchable, @unchecked Sen func rollbackTransaction() async throws {} func switchSchema(to schema: String) async throws { + await onSwitchSchema?() if switchSchemaDelaySeconds > 0 { try await Task.sleep(nanoseconds: UInt64(switchSchemaDelaySeconds * 1_000_000_000)) } diff --git a/TableProTests/Core/Database/SessionSwitchOperationTrackingTests.swift b/TableProTests/Core/Database/SessionSwitchOperationTrackingTests.swift new file mode 100644 index 0000000000..4579bce24e --- /dev/null +++ b/TableProTests/Core/Database/SessionSwitchOperationTrackingTests.swift @@ -0,0 +1,73 @@ +// +// SessionSwitchOperationTrackingTests.swift +// TableProTests +// +// A container switch is a turn on the session driver like any other, so it has to be counted as +// one. `queriesInFlight` is the only thing the health monitor consults before entering the same +// driver alongside the user's work, and holding `sessionDriverGate` does not reach it: the ping +// never asks for that gate. Found alongside #3053. +// + +import Foundation +import Testing + +@testable import TablePro + +@MainActor +private final class Latch { + private var waiters: [CheckedContinuation] = [] + private var isOpen = false + + func open() { + guard !isOpen else { return } + isOpen = true + let pending = waiters + waiters = [] + for waiter in pending { + waiter.resume() + } + } + + func wait() async { + guard !isOpen else { return } + await withCheckedContinuation { waiters.append($0) } + } +} + +@Suite("Session switches count as in-flight work") +@MainActor +struct SessionSwitchOperationTrackingTests { + @Test("A schema switch is in flight while the driver is running it") + func schemaSwitchRegistersAsInFlight() async throws { + let connection = TestFixtures.makeConnection(type: .postgresql) + let driver = MockDatabaseDriver(connection: connection) + driver.currentSchema = "public" + + var session = ConnectionSession(connection: connection, driver: driver) + session.browseSchema = "public" + DatabaseManager.shared.injectSession(session, for: connection.id) + defer { DatabaseManager.shared.removeSession(for: connection.id) } + + #expect(DatabaseManager.shared.queriesInFlight[connection.id] == nil) + + let entered = Latch() + let release = Latch() + driver.onSwitchSchema = { + await entered.open() + await release.wait() + } + + let switching = Task { @MainActor in + try await DatabaseManager.shared.switchSchema(to: "reporting", for: connection.id) + } + await entered.wait() + + #expect(DatabaseManager.shared.queriesInFlight[connection.id] != nil) + + release.open() + try await switching.value + + #expect(DatabaseManager.shared.queriesInFlight[connection.id] == nil) + #expect(driver.currentSchema == "reporting") + } +} diff --git a/TableProTests/Plugins/OracleConnectionErrorTests.swift b/TableProTests/Plugins/OracleConnectionErrorTests.swift index a6d2913e7e..9244a27edb 100644 --- a/TableProTests/Plugins/OracleConnectionErrorTests.swift +++ b/TableProTests/Plugins/OracleConnectionErrorTests.swift @@ -1,6 +1,11 @@ import TableProPluginKit import Testing +/// These cover the copies in `Plugins/TableProPluginKit/`, which no driver calls. The Oracle +/// driver runs the ones in `Packages/TableProOracle/Sources/TableProOracleCore/`, whose own suite +/// is `Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleConnectErrorClassifierTests` +/// and is where the classification that ships is pinned. The two sets have already drifted and +/// nothing forces them to agree; a green run here says nothing about the driver. @Suite("Oracle channel-fatal error classification") struct OracleConnectionErrorTests { @Test("Decode and connection failures are treated as channel-fatal") From c57f4713d05d41d447801656bcca5da0fff0318c Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Tue, 22 Sep 2026 22:52:52 +0700 Subject: [PATCH 2/3] fix(plugin-oracle): replay a session setup only across a close that left the session wanted --- .../OracleConnectErrorClassifier.swift | 26 +++++++++++++-- .../OracleCoreConnection.swift | 27 +++++++++++----- .../OracleDisconnectReason.swift | 17 ++++++++++ .../OracleConnectErrorClassifierTests.swift | 15 ++++++--- .../OracleDisconnectReasonTests.swift | 32 +++++++++++++++++++ .../OracleQueryGateTests.swift | 12 +++++-- .../TableProMobile/Localizable.xcstrings | 6 ++++ 7 files changed, 117 insertions(+), 18 deletions(-) create mode 100644 Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleDisconnectReasonTests.swift diff --git a/Packages/TableProOracle/Sources/TableProOracleCore/OracleConnectErrorClassifier.swift b/Packages/TableProOracle/Sources/TableProOracleCore/OracleConnectErrorClassifier.swift index 188c3b5aef..5890eb306e 100644 --- a/Packages/TableProOracle/Sources/TableProOracleCore/OracleConnectErrorClassifier.swift +++ b/Packages/TableProOracle/Sources/TableProOracleCore/OracleConnectErrorClassifier.swift @@ -87,12 +87,32 @@ public enum OracleChannelFatalCode { } } - /// Whether this side closed the channel, rather than the server or the protocol failing. - public static func isClientClose(_ codeDescription: String) -> Bool { - codeDescription == "clientClosesConnection" || codeDescription == "clientClosedConnection" + /// What took the channel away, for a code ``isChannelFatal(_:serverErrorNumber:)`` calls fatal. + /// + /// The three read very differently to a user. A lost socket and a close from this side are both + /// "the connection went away, run it again"; only a protocol failure is worth telling anyone + /// the server sent something the driver could not read. + public static func closureKind(_ codeDescription: String) -> OracleChannelClosureKind { + switch codeDescription { + case "clientClosesConnection", "clientClosedConnection": + return .clientClose + case "uncleanShutdown", "connectionError": + return .transportLoss + default: + return .protocolFailure + } } } +public enum OracleChannelClosureKind: Sendable, Equatable { + /// This side called `OracleConnection.close()` while the statement was on the wire. + case clientClose + /// The socket went away: the server, a VPN, or the OS closed it. + case transportLoss + /// The driver could not make sense of what came back. + case protocolFailure +} + public enum OracleSSLClassifier { public static func classifyTLSFailure(_ message: String) -> OracleTLSFailureKind? { let lower = message.lowercased() diff --git a/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreConnection.swift b/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreConnection.swift index ef1b336594..db3bec2ebf 100644 --- a/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreConnection.swift +++ b/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreConnection.swift @@ -105,6 +105,7 @@ public final class OracleCoreConnection: @unchecked Sendable { var queryTimeoutSeconds = 0 var sessionSchema: String? var capturesServerOutput = false + var lastCloseReason: OracleDisconnectReason? } private let state = OSAllocatedUnfairLock(initialState: LockedState()) @@ -180,6 +181,7 @@ public final class OracleCoreConnection: @unchecked Sendable { current.sessionID = connectionId current.isConnected = true current.hasEverConnected = true + current.lastCloseReason = nil } osLogger.debug("Connected to Oracle \(self.options.host, privacy: .public):\(self.options.port, privacy: .public)") @@ -284,6 +286,7 @@ public final class OracleCoreConnection: @unchecked Sendable { let connection = state.withLock { current -> OracleNIO.OracleConnection? in guard current.isConnected else { return nil } current.isConnected = false + current.lastCloseReason = reason let connection = current.nioConnection current.nioConnection = nil return connection @@ -401,6 +404,7 @@ public final class OracleCoreConnection: @unchecked Sendable { private func markConnectionDead(reason: OracleDisconnectReason) { let connection = state.withLock { current -> OracleNIO.OracleConnection? in current.isConnected = false + current.lastCloseReason = reason let connection = current.nioConnection current.nioConnection = nil return connection @@ -599,18 +603,23 @@ public final class OracleCoreConnection: @unchecked Sendable { return .queryFailed(serverMessage) } - if OracleChannelFatalCode.isClientClose(code) { + switch OracleChannelFatalCode.closureKind(code) { + case .clientClose: markConnectionDead(reason: .channelAlreadyClosed) osLogger.error("Oracle statement failed because this side had closed the channel: \(code, privacy: .public)") return .connectionClosed + case .transportLoss: + markConnectionDead(reason: .transportError) + osLogger.error("Oracle connection lost during a statement: \(code, privacy: .public)") + return .connectionClosed + case .protocolFailure: + markConnectionDead(reason: .fatalProtocolError) + osLogger.error("Oracle connection reset after a fatal error: \(code, privacy: .public)") + /// ORA-00028 and ORA-00600 end the session, and the server says why better than any + /// wording here could. Everything else that reaches this point is the protocol failing. + guard let serverMessage = sqlError.serverInfo?.message else { return .protocolError } + return .queryFailed(serverMessage) } - - markConnectionDead(reason: .fatalProtocolError) - osLogger.error("Oracle connection reset after a fatal error: \(code, privacy: .public)") - /// ORA-00028 and ORA-00600 end the session, and the server says why better than any - /// wording here could. Everything else that reaches this point is the protocol failing. - guard let serverMessage = sqlError.serverInfo?.message else { return .protocolError } - return .queryFailed(serverMessage) } /// A socket the system reclaimed while the app was suspended surfaces as a @@ -674,6 +683,8 @@ public final class OracleCoreConnection: @unchecked Sendable { do { return try await executeQuery(query) } catch OracleCoreError.connectionClosed { + let closeReason = state.withLock { $0.lastCloseReason } + guard closeReason?.allowsReplay == true else { throw OracleCoreError.connectionClosed } osLogger.notice("Retrying an Oracle session setup statement on a fresh connection") return try await executeQuery(query) } diff --git a/Packages/TableProOracle/Sources/TableProOracleCore/OracleDisconnectReason.swift b/Packages/TableProOracle/Sources/TableProOracleCore/OracleDisconnectReason.swift index da0f3ec24b..a8f53e6a76 100644 --- a/Packages/TableProOracle/Sources/TableProOracleCore/OracleDisconnectReason.swift +++ b/Packages/TableProOracle/Sources/TableProOracleCore/OracleDisconnectReason.swift @@ -17,6 +17,23 @@ public enum OracleDisconnectReason: Sendable, Equatable { case transportError case abandonedLoginAttempt + /// Whether a statement the close killed may be sent again on a replacement connection. + /// + /// A deliberate teardown must never be replayed across. The plugin nils its connection on + /// `disconnect()` and the app removes the session, so a retry would open a socket nobody owns, + /// report a stale schema switch as having succeeded, and leave that session holding none of the + /// state the caller thinks it has. Everything else here took the channel away from a session + /// that is still wanted. + public var allowsReplay: Bool { + switch self { + case .userRequested, .queryCancelled, .abandonedLoginAttempt: + return false + case .queryTimedOut, .pingTimedOut, .wedgedStatement, .channelAlreadyClosed, + .fatalProtocolError, .transportError: + return true + } + } + public var logDescription: String { switch self { case .userRequested: return "the app closed it" diff --git a/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleConnectErrorClassifierTests.swift b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleConnectErrorClassifierTests.swift index 401a98ffaf..0097cde02e 100644 --- a/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleConnectErrorClassifierTests.swift +++ b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleConnectErrorClassifierTests.swift @@ -128,11 +128,16 @@ final class OracleConnectErrorClassifierTests: XCTestCase { XCTAssertFalse(OracleChannelFatalCode.isChannelFatal("server")) } - func testClientClosesAreToldApartFromProtocolFailures() { - XCTAssertTrue(OracleChannelFatalCode.isClientClose("clientClosedConnection")) - XCTAssertTrue(OracleChannelFatalCode.isClientClose("clientClosesConnection")) - XCTAssertFalse(OracleChannelFatalCode.isClientClose("connectionError")) - XCTAssertFalse(OracleChannelFatalCode.isClientClose("uncleanShutdown")) + /// A lost socket must not be reported as the server sending something the driver could not + /// read: `uncleanShutdown` and `connectionError` are the transport going away, and + /// `OracleConnectErrorClassifier` already calls the first of them a dropped connection. + func testClosuresAreToldApartByWhatTookTheChannel() { + XCTAssertEqual(OracleChannelFatalCode.closureKind("clientClosedConnection"), .clientClose) + XCTAssertEqual(OracleChannelFatalCode.closureKind("clientClosesConnection"), .clientClose) + XCTAssertEqual(OracleChannelFatalCode.closureKind("uncleanShutdown"), .transportLoss) + XCTAssertEqual(OracleChannelFatalCode.closureKind("connectionError"), .transportLoss) + XCTAssertEqual(OracleChannelFatalCode.closureKind("messageDecodingFailure"), .protocolFailure) + XCTAssertEqual(OracleChannelFatalCode.closureKind("unexpectedBackendMessage"), .protocolFailure) } func testTLSClassifierRecognizesOracleWalletAndCipherErrors() { diff --git a/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleDisconnectReasonTests.swift b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleDisconnectReasonTests.swift new file mode 100644 index 0000000000..a6a954a976 --- /dev/null +++ b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleDisconnectReasonTests.swift @@ -0,0 +1,32 @@ +@testable import TableProOracleCore +import XCTest + +/// A statement may only be sent again across a close that took the channel away from a session +/// somebody still wants. Replaying across a deliberate teardown opens a socket nobody owns, and +/// reports a switch the caller has already abandoned as having succeeded. +final class OracleDisconnectReasonTests: XCTestCase { + func testADeliberateTeardownIsNeverReplayedAcross() { + XCTAssertFalse(OracleDisconnectReason.userRequested.allowsReplay) + XCTAssertFalse(OracleDisconnectReason.queryCancelled.allowsReplay) + XCTAssertFalse(OracleDisconnectReason.abandonedLoginAttempt.allowsReplay) + } + + func testAChannelTakenFromALiveSessionMayBeReplayedAcross() { + XCTAssertTrue(OracleDisconnectReason.pingTimedOut.allowsReplay) + XCTAssertTrue(OracleDisconnectReason.queryTimedOut.allowsReplay) + XCTAssertTrue(OracleDisconnectReason.wedgedStatement.allowsReplay) + XCTAssertTrue(OracleDisconnectReason.channelAlreadyClosed.allowsReplay) + XCTAssertTrue(OracleDisconnectReason.transportError.allowsReplay) + XCTAssertTrue(OracleDisconnectReason.fatalProtocolError.allowsReplay) + } + + func testEveryReasonSaysSomethingTheLogCanUse() { + let reasons: [OracleDisconnectReason] = [ + .userRequested, .queryCancelled, .queryTimedOut, .pingTimedOut, .wedgedStatement, + .channelAlreadyClosed, .fatalProtocolError, .transportError, .abandonedLoginAttempt + ] + for reason in reasons { + XCTAssertFalse(reason.logDescription.isEmpty, String(describing: reason)) + } + } +} diff --git a/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleQueryGateTests.swift b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleQueryGateTests.swift index c6a6e1e333..ee32ffea37 100644 --- a/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleQueryGateTests.swift +++ b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleQueryGateTests.swift @@ -37,19 +37,27 @@ final class OracleQueryGateTests: XCTestCase { await gate.release() } + /// Compared against the first holder's own age rather than a wall-clock bound: time only moves + /// forward, so without the reset the second reading could never be the smaller one, whatever a + /// loaded CI worker does to the scheduling in between. func testAHandoverRestartsTheHoldingClock() async { let gate = QueryGate() await gate.acquire() let queued = Task { await gate.acquire() } try? await Task.sleep(for: .milliseconds(120)) + guard case .busy(let beforeHandover) = await gate.takeTurnIfFree() else { + return XCTFail("The first holder still holds the gate") + } + await gate.release() await queued.value - guard case .busy(let held) = await gate.takeTurnIfFree() else { + guard case .busy(let afterHandover) = await gate.takeTurnIfFree() else { return XCTFail("The waiter now holds the gate") } - XCTAssertLessThan(held, .milliseconds(100)) + XCTAssertGreaterThan(beforeHandover, .zero) + XCTAssertLessThan(afterHandover, beforeHandover) await gate.release() } } diff --git a/TableProMobile/TableProMobile/Localizable.xcstrings b/TableProMobile/TableProMobile/Localizable.xcstrings index 4e71332ea7..827a6133bb 100644 --- a/TableProMobile/TableProMobile/Localizable.xcstrings +++ b/TableProMobile/TableProMobile/Localizable.xcstrings @@ -21016,6 +21016,12 @@ }, "Read it with a command in Query." : { + }, + "The Oracle connection closed while the statement was running. Run it again." : { + + }, + "The Oracle driver reported an error (%@)." : { + } }, "version" : "1.0" From 447f8dc26bca1ed1cdddd1e40333059442326714 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Tue, 22 Sep 2026 23:10:39 +0700 Subject: [PATCH 3/3] fix(plugin-oracle): let nothing redial an Oracle connection the app has disconnected --- .../OracleCloseRecord.swift | 40 +++++++++++++ .../OracleCoreConnection.swift | 32 +++++++--- .../OracleDisconnectReason.swift | 12 ++++ .../OracleCloseRecordTests.swift | 60 +++++++++++++++++++ .../OracleDisconnectReasonTests.swift | 12 ++++ 5 files changed, 148 insertions(+), 8 deletions(-) create mode 100644 Packages/TableProOracle/Sources/TableProOracleCore/OracleCloseRecord.swift create mode 100644 Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleCloseRecordTests.swift diff --git a/Packages/TableProOracle/Sources/TableProOracleCore/OracleCloseRecord.swift b/Packages/TableProOracle/Sources/TableProOracleCore/OracleCloseRecord.swift new file mode 100644 index 0000000000..7a112f2d2b --- /dev/null +++ b/Packages/TableProOracle/Sources/TableProOracleCore/OracleCloseRecord.swift @@ -0,0 +1,40 @@ +import Foundation + +/// What is known about the last time this connection's channel went away, and whether the +/// connection is finished for good. +/// +/// Two closers reach one channel: whoever decided to close it, and the statement that was on the +/// wire when it went. The second one arrives through OracleNIO as `clientClosedConnection` and +/// knows nothing about the first, so it must not be allowed to overwrite what the first recorded. +/// Letting it did exactly that: a user disconnect recorded "the app closed it", the dying +/// statement replaced it with "OracleNIO had already closed the channel", and the replay guard +/// then read a reason that permits a redial. +/// +/// `isFinished` is deliberately one-way. The plugin drops its `OracleCoreConnection` when the app +/// disconnects and builds a new one to reconnect, so a connection closed that way is never reached +/// again by anything the app owns, and anything still holding it has to find it finished. +internal struct OracleCloseRecord: Sendable, Equatable { + private(set) var reason: OracleDisconnectReason? + private(set) var isFinished = false + + mutating func record(_ reason: OracleDisconnectReason) { + isFinished = isFinished || reason.endsConnection + guard self.reason == nil else { return } + self.reason = reason + } + + mutating func clearOnConnect() { + reason = nil + } + + /// Whether a statement that only configures the session may be sent again on a replacement + /// connection. + var allowsSessionSetupReplay: Bool { + !isFinished && reason?.allowsReplay == true + } + + /// Whether a statement that finds no channel may open one. + var allowsReconnect: Bool { + !isFinished + } +} diff --git a/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreConnection.swift b/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreConnection.swift index db3bec2ebf..588c86258b 100644 --- a/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreConnection.swift +++ b/Packages/TableProOracle/Sources/TableProOracleCore/OracleCoreConnection.swift @@ -105,7 +105,7 @@ public final class OracleCoreConnection: @unchecked Sendable { var queryTimeoutSeconds = 0 var sessionSchema: String? var capturesServerOutput = false - var lastCloseReason: OracleDisconnectReason? + var close = OracleCloseRecord() } private let state = OSAllocatedUnfairLock(initialState: LockedState()) @@ -176,12 +176,23 @@ public final class OracleCoreConnection: @unchecked Sendable { return connection } - state.withLock { current in + /// A dial the app gave up on while it was in flight has nowhere to land: the plugin + /// dropped this connection and built another, so installing the handle here would + /// leave a session open on the server that nothing can reach or close. + let adopted = state.withLock { current -> Bool in + guard current.close.allowsReconnect else { return false } current.nioConnection = connection current.sessionID = connectionId current.isConnected = true current.hasEverConnected = true - current.lastCloseReason = nil + current.close.clearOnConnect() + return true + } + + guard adopted else { + try? await connection.close() + osLogger.notice("Closed an Oracle connection that finished dialing after the app let it go") + throw OracleCoreError.notConnected } osLogger.debug("Connected to Oracle \(self.options.host, privacy: .public):\(self.options.port, privacy: .public)") @@ -284,9 +295,9 @@ public final class OracleCoreConnection: @unchecked Sendable { public func disconnect(reason: OracleDisconnectReason = .userRequested) { let connection = state.withLock { current -> OracleNIO.OracleConnection? in + current.close.record(reason) guard current.isConnected else { return nil } current.isConnected = false - current.lastCloseReason = reason let connection = current.nioConnection current.nioConnection = nil return connection @@ -404,7 +415,7 @@ public final class OracleCoreConnection: @unchecked Sendable { private func markConnectionDead(reason: OracleDisconnectReason) { let connection = state.withLock { current -> OracleNIO.OracleConnection? in current.isConnected = false - current.lastCloseReason = reason + current.close.record(reason) let connection = current.nioConnection current.nioConnection = nil return connection @@ -427,7 +438,11 @@ public final class OracleCoreConnection: @unchecked Sendable { if let connection = state.withLock({ $0.isConnected ? $0.nioConnection : nil }) { return connection } - guard state.withLock({ $0.hasEverConnected }) else { + /// A statement that was queued behind the gate when the app disconnected must find this + /// connection finished. Without this it redials instead, and the socket it opens belongs to + /// nobody: the plugin has already dropped this connection and the app has removed the + /// session, so nothing will ever close it. + guard state.withLock({ $0.hasEverConnected && $0.close.allowsReconnect }) else { throw OracleCoreError.notConnected } @@ -683,8 +698,9 @@ public final class OracleCoreConnection: @unchecked Sendable { do { return try await executeQuery(query) } catch OracleCoreError.connectionClosed { - let closeReason = state.withLock { $0.lastCloseReason } - guard closeReason?.allowsReplay == true else { throw OracleCoreError.connectionClosed } + guard state.withLock({ $0.close.allowsSessionSetupReplay }) else { + throw OracleCoreError.connectionClosed + } osLogger.notice("Retrying an Oracle session setup statement on a fresh connection") return try await executeQuery(query) } diff --git a/Packages/TableProOracle/Sources/TableProOracleCore/OracleDisconnectReason.swift b/Packages/TableProOracle/Sources/TableProOracleCore/OracleDisconnectReason.swift index a8f53e6a76..96fac81d6a 100644 --- a/Packages/TableProOracle/Sources/TableProOracleCore/OracleDisconnectReason.swift +++ b/Packages/TableProOracle/Sources/TableProOracleCore/OracleDisconnectReason.swift @@ -34,6 +34,18 @@ public enum OracleDisconnectReason: Sendable, Equatable { } } + /// Whether this close ends the connection for good, rather than taking a channel away from a + /// session that still wants one. + /// + /// The plugin drops its `OracleCoreConnection` when the app disconnects and builds a new one to + /// reconnect, so a connection closed this way is never reached again by anything the app owns. + /// Anything that still holds it, a statement queued behind the gate or a retry dial already in + /// flight, must find it finished rather than quietly opening a second socket on the server. + /// Cancelling a query is not this: it ends one statement, and the session goes on. + public var endsConnection: Bool { + self == .userRequested + } + public var logDescription: String { switch self { case .userRequested: return "the app closed it" diff --git a/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleCloseRecordTests.swift b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleCloseRecordTests.swift new file mode 100644 index 0000000000..3d06c4ebb5 --- /dev/null +++ b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleCloseRecordTests.swift @@ -0,0 +1,60 @@ +@testable import TableProOracleCore +import XCTest + +/// Two closers reach one channel: whoever decided to close it, and the statement that was on the +/// wire when it went. The second arrives as `clientClosedConnection` and knows nothing about the +/// first, so it must not be able to talk the connection into redialing after the app let it go. +final class OracleCloseRecordTests: XCTestCase { + func testAFreshRecordPermitsBothRecoveries() { + var record = OracleCloseRecord() + XCTAssertTrue(record.allowsReconnect) + XCTAssertFalse(record.allowsSessionSetupReplay) + + record.record(.pingTimedOut) + XCTAssertTrue(record.allowsReconnect) + XCTAssertTrue(record.allowsSessionSetupReplay) + } + + func testTheDyingStatementCannotOverwriteADeliberateTeardown() { + var record = OracleCloseRecord() + record.record(.userRequested) + record.record(.channelAlreadyClosed) + + XCTAssertEqual(record.reason, .userRequested) + XCTAssertFalse(record.allowsSessionSetupReplay) + XCTAssertFalse(record.allowsReconnect) + } + + func testATeardownArrivingSecondStillFinishesTheConnection() { + var record = OracleCloseRecord() + record.record(.transportError) + record.record(.userRequested) + + XCTAssertEqual(record.reason, .transportError) + XCTAssertFalse(record.allowsSessionSetupReplay) + XCTAssertFalse(record.allowsReconnect) + } + + func testCancellingAQueryDoesNotFinishTheConnection() { + var record = OracleCloseRecord() + record.record(.queryCancelled) + + XCTAssertTrue(record.allowsReconnect) + XCTAssertFalse(record.allowsSessionSetupReplay) + } + + /// A connect clears the reason so the next failure reads its own, and leaves `isFinished` + /// alone: the plugin never reuses a connection the app disconnected. + func testConnectingClearsTheReasonButNotTheTeardown() { + var record = OracleCloseRecord() + record.record(.transportError) + record.clearOnConnect() + XCTAssertNil(record.reason) + XCTAssertTrue(record.allowsReconnect) + XCTAssertFalse(record.allowsSessionSetupReplay) + + record.record(.userRequested) + record.clearOnConnect() + XCTAssertFalse(record.allowsReconnect) + } +} diff --git a/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleDisconnectReasonTests.swift b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleDisconnectReasonTests.swift index a6a954a976..805556e5aa 100644 --- a/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleDisconnectReasonTests.swift +++ b/Packages/TableProOracle/Tests/TableProOracleCoreTests/OracleDisconnectReasonTests.swift @@ -20,6 +20,18 @@ final class OracleDisconnectReasonTests: XCTestCase { XCTAssertTrue(OracleDisconnectReason.fatalProtocolError.allowsReplay) } + /// Only the app's own disconnect ends the connection. Cancelling a query ends one statement, + /// and an abandoned login attempt closes its own handle without touching the one installed. + func testOnlyTheAppsOwnDisconnectEndsTheConnection() { + XCTAssertTrue(OracleDisconnectReason.userRequested.endsConnection) + for reason in [ + OracleDisconnectReason.queryCancelled, .queryTimedOut, .pingTimedOut, .wedgedStatement, + .channelAlreadyClosed, .fatalProtocolError, .transportError, .abandonedLoginAttempt + ] { + XCTAssertFalse(reason.endsConnection, String(describing: reason)) + } + } + func testEveryReasonSaysSomethingTheLogCanUse() { let reasons: [OracleDisconnectReason] = [ .userRequested, .queryCancelled, .queryTimedOut, .pingTimedOut, .wedgedStatement,