fix(plugin-oracle): stop infinite loading when switching schemas#1811
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69cee7c266
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch let timeout as TimeoutError { | ||
| throw queryTimeoutError(timeout) |
There was a problem hiding this comment.
Release the Oracle query gate on timeout
When an Oracle query times out while another operation has already passed requireConnection() and is waiting in queryGate.acquire() (for example a schema reload or a second tab query), this timeout branch throws without releasing the gate. Because withQueryDeadline closes the connection and then throws TimeoutError, the queued waiter is never resumed, leaving that operation stuck indefinitely instead of failing/reconnecting; the streaming timeout branch has the same missing release.
Useful? React with 👍 / 👎.
…ly, and normalize legacy metadata (#1807)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f37b876afd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| static let brandColorHex = "#C3160B" | ||
| static let systemDatabaseNames: [String] = ["SYS", "SYSTEM", "OUTLN", "DBSNMP", "APPQOSSYS", "WMSYS", "XDB"] | ||
| static let databaseGroupingStrategy: GroupingStrategy = .bySchema | ||
| static let databaseGroupingStrategy: GroupingStrategy = .hierarchicalSchema |
There was a problem hiding this comment.
Move Oracle system schemas into schema metadata
After switching Oracle to .hierarchicalSchema, the UI paths for hierarchical/schema-only containers filter and mark system entries via systemSchemaNames (e.g. the sidebar tree and schema switcher), not systemDatabaseNames. Since Oracle's SYS/SYSTEM/etc. list is still only declared as systemDatabaseNames while fetchSchemas() returns ALL_USERS, those accounts will now appear as ordinary selectable schemas and won't be hidden behind the existing system-schema controls; the mirrored registry default needs the same migration.
Useful? React with 👍 / 👎.
Fixes #1807.
Root cause
Three layered problems produced the infinite spinner:
OraclePluginnever overrodesupportsDatabaseSwitching(protocol defaulttrue) ordefaultSchemaName(default"public").containerSwitchTargettherefore returned.database, so the switcher routed throughswitchDatabase, whose.bySchemabranch resetsession.currentSchemato"public", a schema that does not exist on any Oracle server. Oracle has one tier: a schema is a user and is the only switchable container.MetadataConnectionPoolkey. The pool opened a fresh Oracle connection and ran the schema switch with no bound. Oracle was the only plugin implementing no query timeout, and itsQueryGateserializes all queries, so one stuck statement blocked the connection forever.reloadSchema's catch block only logged, soSchemaServicenever left.loadingwhen driver acquisition failed.Fix
Plugin side (
Plugins/OracleDriverPlugin/, ships via the nextplugin-oracle-v*release, no PluginKit ABI change):supportsDatabaseSwitching = false,databaseGroupingStrategy = .hierarchicalSchema,defaultSchemaName = "",containerEntityName = "Schema". The sidebar becomes the hierarchical schema tree (schemas with lazily loaded tables). The old two-level tree ran the sameALL_USERSquery at both levels and showed the identical list twice.applyQueryTimeout. Queries race the configured timeout (Settings > General, 60s default). On timeout the connection is closed first, which fails the in-flight OracleNIO call even if it ignores task cancellation, then the query gate is released so queued queries (including the health ping) fail fast instead of hanging.queryTimedOuterror category with a diagnostic (retry, raise the timeout, busy-server hint).cancelQueryimplementation: the app cancels queries in supersede style (refresh, pagination, new run) and expects the connection to survive, which Oracle cannot honor without a server-side cancel. The timeout bounds stuck queries instead.App side (ships with the next app release):
MetadataConnectionPoolbounds the post-connect schema switch (15s) for every plugin. The sharedwithTimeouthelper gained anonTimeouthook (additive PluginKit overload) that force-disconnects the driver, so a driver that ignores task cancellation still unwinds; the pool and the Oracle wrapper share one race implementation.SchemaService.markLoadFailedsurfaces.failedwhen driver acquisition fails (without clobbering already-loaded tables); a failed schema-list fetch on the hierarchical path stamps.failedinstead of rendering an empty tree; the sidebar error state gained a Retry button, and Retry reports a clear failure when the session is unavailable instead of silently doing nothing.hierarchicalSchema+supportsDatabaseSwitchingpair.Deliberately unchanged:
DatabaseManager.switchDatabase's.bySchemaschema reset. MSSQL depends on it (its driver never updates its own schema on a database switch), and a new test locks that in.Version skew
applyQueryTimeoutgenerically.No
currentPluginKitVersionbump: thewithTimeout(onTimeout:)overload is a new symbol (additive), and every other touched API already exists with defaults.Tests
ContainerEntityNameTests: Oracle targets.schema, no database switching,Schemacontainer, hierarchical grouping, empty default schema.PluginMetadataSwitchRoutingTests(new): legacy two-tier declarations are detected against a schema-only registry default, and normalization carries over exactly the four routing fields.SwitchContainerTests(new): coordinator-level regression provingswitchContainer(to: "HR")on an Oracle connection routes toswitchSchemaand updates toolbar and session state.DatabaseManagerDatabaseSwitchTests(new): MSSQL database switch still resets the session schema todbo.MetadataConnectionPoolTests(new): hanging connect and schema switch fail within the deadline; a driver that ignores cancellation is force-disconnected and still fails in time; fast paths pass through.AsyncTimeoutTests:onTimeoutunblocks a cancellation-deaf operation and is not invoked when the operation wins.SchemaServiceTests:markLoadFailedsemantics, hierarchical schema list success and failure states, and Retry-without-session feedback.No UI automation: the flow needs a live Oracle server, which the UI test runner does not have. The Oracle wrapper's timeout and reconnect internals need a manual pass against a real server (
gvenzl/oracle-xe:21-slim).Docs
docs/customization/settings.mdx: documents Oracle's client-side timeout enforcement (connection reset + auto-reconnect).docs/databases/oracle.mdx: describes the hierarchical schema sidebar.Manual verification checklist