Skip to content

fix(plugin-oracle): stop a health check closing a connection a statement is running on - #3055

Merged
datlechin merged 3 commits into
mainfrom
fix/3053-oracle-client-closed-connection
Sep 22, 2026
Merged

datlechin merged 3 commits into
mainfrom
fix/3053-oracle-client-closed-connection

Conversation

@datlechin

@datlechin datlechin commented Sep 22, 2026

Copy link
Copy Markdown
Member

Fixes #3053.

What the reported error means

oracle-nio raises OracleSQLError(code: clientClosedConnection) from exactly one place:
ConnectionStateMachine.close(_:), i.e. when the client calls OracleConnection.close().
closeConnectionAndCleanup then fails every in-flight task with it. So the reporter's error says
TablePro 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:246 and :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() wrapped executeQuery in a 10-second deadline whose onTimeout
called disconnect(). executeQuery's first act is await queryGate.acquire(), so the deadline
covered the queue wait, not the round trip. A probe that never got a turn closed the channel
anyway.

Measured, with the repo's own QueryGate and withOracleTimeout copied verbatim into a swiftc
harness (times scaled 10x down so it runs in seconds):

=== BEFORE: ping() = withOracleTimeout(10) { executeQuery } ===
  [ 0.00s] statement: running on the channel
  [ 1.17s] ping onTimeout -> disconnect() closed the channel
  [ 1.59s] statement: finished, channel alive: false
  [ 1.59s] ping: got its turn at last
  RESULT: ping=timed out, the statement kept a live channel: false

=== AFTER: ping() takes a turn first, and only closes a channel it owns ===
  [ 1.59s] statement: running on the channel
  [ 1.70s] ping: the channel is in use, which answers the question. Skipped.
  [ 3.10s] statement: finished, channel alive: true
  RESULT: ping=reported alive, the statement kept a live channel: true

2. A closed channel was invisible, and the driver's own struct dump was what the user saw.
OracleChannelFatalCode.isChannelFatal listed three codes where oracle-nio's own
ConnectionStateMachine.shouldCloseConnection(reason:) lists fourteen plus ORA-28 and ORA-600.
clientClosedConnection was not among our three, so mapQueryError neither marked the connection
dead nor translated anything: it returned .queryFailed(sqlError.description). That description is
character-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
OracleCoreConnection was 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. connectToSession calls markSessionVerified and ConnectionHealthCheck.freshness is 300s,
so verifyBeforeUse returns early; ConnectionHealthMonitor.startMonitoring sleeps a random 0-10s
and 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.

  • QueryGate gains takeTurnIfFree(), which either hands over the channel or says how long
    somebody 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 runs SELECT 1 through the statement path. It uses oracle-nio's own
    OracleConnection.ping() and isClosed, so there is no transaction role to admit and no silent
    redial: 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 wedged
    instead, which is the app's own staleness rule (DatabaseManager+Health.swift) and keeps its
    escape valve working.
  • OracleChannelFatalCode mirrors oracle-nio's shouldCloseConnection, which is internal and
    so can only be mirrored, including both client-close codes and ORA-28 / ORA-600.
  • mapQueryError never puts OracleSQLError.description into a user-facing string. The
    server'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.connectionClosed with a real sentence and a
    PluginDiagnostic with suggested actions.
  • disconnect(reason:) and markConnectionDead(reason:) log why, so the next report names its
    closer.
  • OraclePluginDriver.switchSchema retries once across a lost channel, through a new
    executeSessionSetup. Narrow on purpose: a session-setup statement is one reconnectedConnection()
    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.admit rather than carrying on. It is not generalised to arbitrary
    statements.
  • OracleCloseRecord owns 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 clientClosedConnection and knows nothing about the first, so the first reason
    recorded 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 OracleCoreConnection on disconnect and builds a new one to
    reconnect, so that flag is one-way by construction.
  • DatabaseManager.switchSchema and the in-place arm of switchDatabase now register through
    trackOperation, like withSessionDriverTurn and reconnectOntoDatabase already do. A separate
    confirmed gap in the same class, not part of [Oracle] Schema switch fails with clientClosedConnection on a healthy connection #3053's causal chain: holding sessionDriverGate
    never reached queriesInFlight, which is the only thing the ping guard consults.

Verified

Step Result
swift test --package-path Packages/TableProOracle 153 executed, 0 failed
verify.sh build OracleDriver PASS
verify.sh build (app) PASS
verify.sh test on 11 affected suites 78 executed, 78 passed
swiftlint --strict on every changed file 0 violations
before/after probe above

Suites run: SessionSwitchOperationTrackingTests, SwitchSchemaTests, SwitchContainerTests,
DatabaseManagerTests, ConnectionVerificationTests, ProtectedWritePingSuppressionTests,
HealthMonitorReconnectTests, OracleConnectionErrorTests, SQLSchemaProviderTests,
DatabaseSwitchLeaseOrderingTests, DatabaseManagerSchemaChangeRoutingTests.

New tests: OraclePingDecisionTests, OracleQueryGateTests, OracleCloseRecordTests,
OracleDisconnectReasonTests, OracleCoreErrorMessageTests (asserts no OracleCoreError message
can contain OracleSQLError( or triggeredFromRequestInFile), the shouldCloseConnection mirror
in OracleConnectErrorClassifierTests, and SessionSwitchOperationTrackingTests. The Oracle
package suite went from 144 to 153 cases.

Reviewed by Codex (review and adversarial-review), and both rounds changed the code. The first
found that the replay was unconditional, that uncleanShutdown was being reported as protocol
corruption, 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 markConnectionDead overwriting the recorded reason, which is what OracleCloseRecord
and its five tests now pin.

No TableProPluginKit change, so no currentPluginKitVersion bump and no ABI check needed. Oracle is
registry-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 TableProUITests can arrange. The behaviour is pinned by unit tests on
the decision, the gate and the error messages instead.

Note on OracleConnectionErrorTests

Plugins/TableProPluginKit/ holds three Oracle classifier copies with no production caller, and the
app'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.

@datlechin
datlechin merged commit 805131b into main Sep 22, 2026
11 of 13 checks passed
@datlechin
datlechin deleted the fix/3053-oracle-client-closed-connection branch September 22, 2026 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Oracle] Schema switch fails with clientClosedConnection on a healthy connection

1 participant