fix(plugin-oracle): stop a health check closing a connection a statement is running on - #3055
Merged
Merged
Conversation
…ent is running on
…eft the session wanted
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3053.
What the reported error means
oracle-nio raises
OracleSQLError(code: clientClosedConnection)from exactly one place:ConnectionStateMachine.close(_:), i.e. when the client callsOracleConnection.close().closeConnectionAndCleanupthen fails every in-flight task with it. So the reporter's error saysTablePro closed the channel out from under its own running statement, on a connection that was
healthy.
Read at the revision we pin (
ccebdb59):ConnectionStateMachine.swift:246and:1083,OracleChannelHandler.swift:922-941.Root cause
TablePro closed an Oracle channel from code that did not own the statement on it, and then did not
recognise the error that produces.
1. A health check could close a channel it never got a turn on.
OracleCoreConnection.ping()wrappedexecuteQueryin a 10-second deadline whoseonTimeoutcalled
disconnect().executeQuery's first act isawait queryGate.acquire(), so the deadlinecovered the queue wait, not the round trip. A probe that never got a turn closed the channel
anyway.
Measured, with the repo's own
QueryGateandwithOracleTimeoutcopied verbatim into aswiftcharness (times scaled 10x down so it runs in seconds):
2. A closed channel was invisible, and the driver's own struct dump was what the user saw.
OracleChannelFatalCode.isChannelFatallisted three codes where oracle-nio's ownConnectionStateMachine.shouldCloseConnection(reason:)lists fourteen plus ORA-28 and ORA-600.clientClosedConnectionwas not among our three, somapQueryErrorneither marked the connectiondead nor translated anything: it returned
.queryFailed(sqlError.description). That description ischaracter-for-character the text in the reporter's dialog, and oracle-nio's own doc comment says
these errors "should not be forwareded to the end user, as they may leak sensitive information"
(
OracleSQLError.swift:25).What this does not claim
Which closer fired for this reporter is not identified. Every client-side closer in
OracleCoreConnectionwas enumerated, and the obvious candidate is ruled out for their timeline:the error arrived ~17.5s after the sockets were ready, but no ping of either kind can be in flight
then.
connectToSessioncallsmarkSessionVerifiedandConnectionHealthCheck.freshnessis 300s,so
verifyBeforeUsereturns early;ConnectionHealthMonitor.startMonitoringsleeps a random 0-10sand then a full interval, and the shortest selectable interval is 30s.
The ping race is real and reachable on any session older than ~30 seconds, and it is removed here.
For whatever else closed that channel, the fix still applies: the error is now classified, the
connection is marked dead so the next statement redials, the message is one a user can act on, and
every close names itself in the log so the next report identifies its closer instead of leaving it
to inference.
The change
Only the task using a channel may close it, and a deadline may only cover work that has the
channel.
QueryGategainstakeTurnIfFree(), which either hands over the channel or says how longsomebody else has held it. This is Sequel Ace's rule: "Attempt to lock the connection. If the
connection is currently busy, we don't need a ping."
ping()no longer runsSELECT 1through the statement path. It uses oracle-nio's ownOracleConnection.ping()andisClosed, so there is no transaction role to admit and no silentredial: a connection that has gone away now reports that, instead of reporting the health of a
replacement nobody asked for. A busy channel reads as alive, because the statement on it answers
the question better than a probe could. Past
max(queryTimeout, 300)seconds it reads as wedgedinstead, which is the app's own staleness rule (
DatabaseManager+Health.swift) and keeps itsescape valve working.
OracleChannelFatalCodemirrors oracle-nio'sshouldCloseConnection, which is internal andso can only be mirrored, including both client-close codes and ORA-28 / ORA-600.
mapQueryErrornever putsOracleSQLError.descriptioninto a user-facing string. Theserver's own message when there is one, the code's name otherwise; the full description goes to
OSLog. A client close maps to a new
OracleCoreError.connectionClosedwith a real sentence and aPluginDiagnosticwith suggested actions.disconnect(reason:)andmarkConnectionDead(reason:)log why, so the next report names itscloser.
OraclePluginDriver.switchSchemaretries once across a lost channel, through a newexecuteSessionSetup. Narrow on purpose: a session-setup statement is onereconnectedConnection()already replays on every reconnect, so running it again is what the connection would have done
anyway, and a transaction bound to the closed session still fails at
OracleSessionTransaction.admitrather than carrying on. It is not generalised to arbitrarystatements.
OracleCloseRecordowns what is known about a close, because 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
clientClosedConnectionand knows nothing about the first, so the first reasonrecorded is the one that stands. A close the app asked for also finishes the connection for good:
nothing redials it, no dial still in flight installs its handle, and no queued statement replays
across it. The plugin drops its
OracleCoreConnectionon disconnect and builds a new one toreconnect, so that flag is one-way by construction.
DatabaseManager.switchSchemaand the in-place arm ofswitchDatabasenow register throughtrackOperation, likewithSessionDriverTurnandreconnectOntoDatabasealready do. A separateconfirmed gap in the same class, not part of [Oracle] Schema switch fails with clientClosedConnection on a healthy connection #3053's causal chain: holding
sessionDriverGatenever reached
queriesInFlight, which is the only thing the ping guard consults.Verified
swift test --package-path Packages/TableProOracleverify.sh build OracleDriververify.sh build(app)verify.sh teston 11 affected suitesswiftlint --stricton every changed fileSuites run:
SessionSwitchOperationTrackingTests,SwitchSchemaTests,SwitchContainerTests,DatabaseManagerTests,ConnectionVerificationTests,ProtectedWritePingSuppressionTests,HealthMonitorReconnectTests,OracleConnectionErrorTests,SQLSchemaProviderTests,DatabaseSwitchLeaseOrderingTests,DatabaseManagerSchemaChangeRoutingTests.New tests:
OraclePingDecisionTests,OracleQueryGateTests,OracleCloseRecordTests,OracleDisconnectReasonTests,OracleCoreErrorMessageTests(asserts noOracleCoreErrormessagecan contain
OracleSQLError(ortriggeredFromRequestInFile), theshouldCloseConnectionmirrorin
OracleConnectErrorClassifierTests, andSessionSwitchOperationTrackingTests. The Oraclepackage suite went from 144 to 153 cases.
Reviewed by Codex (
reviewandadversarial-review), and both rounds changed the code. The firstfound that the replay was unconditional, that
uncleanShutdownwas being reported as protocolcorruption, that the two shared messages were missing from the iOS catalog, and that a gate test
carried a wall-clock bound. The second found that the replay guard added for the first was itself
defeated by
markConnectionDeadoverwriting the recorded reason, which is whatOracleCloseRecordand its five tests now pin.
No TableProPluginKit change, so no
currentPluginKitVersionbump and no ABI check needed. Oracle isregistry-only, so users reach this through a
plugin-oracle-v*release.Screenshots
None. The failure needs a live Oracle server and a race to reproduce, so neither the before nor the
after state can be captured here; the before is the dialog in the issue. The strings the change
introduces are quoted above.
Deterministic UI automation
Not added. Reproducing this needs a live Oracle server and a race between a health check and a
statement, neither of which
TableProUITestscan arrange. The behaviour is pinned by unit tests onthe decision, the gate and the error messages instead.
Note on
OracleConnectionErrorTestsPlugins/TableProPluginKit/holds three Oracle classifier copies with no production caller, and theapp's test suite imports those. They are left alone: removing public symbols from a
Library-Evolution framework is an ABI removal. The suite gained a doc comment saying it covers the
dead copies and naming the package suite that pins what ships. The duplication itself is reported
separately.