From ead3fe2e73e38cc6d7cde871f6fbc9a6ddbad254 Mon Sep 17 00:00:00 2001 From: sudosubin Date: Wed, 22 Jul 2026 01:56:38 +0900 Subject: [PATCH] fix: trigger failover on read-only connection errors in strict-writer mode --- common/lib/error_handler.ts | 3 + common/lib/plugin_service.ts | 4 ++ .../lib/plugins/failover/failover_plugin.ts | 8 ++- .../lib/plugins/failover2/failover2_plugin.ts | 8 ++- mysql/lib/mysql_error_handler.ts | 9 +++ pg/lib/abstract_pg_error_handler.ts | 9 +++ tests/unit/error_handler.test.ts | 55 +++++++++++++++++++ tests/unit/failover2_plugin.test.ts | 52 ++++++++++++++++++ tests/unit/failover_plugin.test.ts | 52 ++++++++++++++++++ 9 files changed, 198 insertions(+), 2 deletions(-) create mode 100644 tests/unit/error_handler.test.ts diff --git a/common/lib/error_handler.ts b/common/lib/error_handler.ts index 305ddbc8c..8469d5216 100644 --- a/common/lib/error_handler.ts +++ b/common/lib/error_handler.ts @@ -23,6 +23,9 @@ export interface ErrorHandler { isSyntaxError(e: Error): boolean; + // True when a write was attempted on a read-only connection, e.g. a writer demoted after failover. + isReadOnlyConnectionError(e: Error): boolean; + /** * Checks whether there has been an unexpected error emitted and if the error is a type of login error. */ diff --git a/common/lib/plugin_service.ts b/common/lib/plugin_service.ts index e2aae5062..8f64346c8 100644 --- a/common/lib/plugin_service.ts +++ b/common/lib/plugin_service.ts @@ -702,6 +702,10 @@ export class PluginServiceImpl implements PluginService, HostListProviderService return this.getDialect().getErrorHandler().isSyntaxError(e); } + isReadOnlyConnectionError(e: Error): boolean { + return this.getDialect().getErrorHandler().isReadOnlyConnectionError(e); + } + hasLoginError(): boolean { return this.getDialect().getErrorHandler().hasLoginError(); } diff --git a/common/lib/plugins/failover/failover_plugin.ts b/common/lib/plugins/failover/failover_plugin.ts index e6d7f4d75..a17b713ce 100644 --- a/common/lib/plugins/failover/failover_plugin.ts +++ b/common/lib/plugins/failover/failover_plugin.ts @@ -522,7 +522,13 @@ export class FailoverPlugin extends AbstractConnectionPlugin { } if (error instanceof Error) { - return this.pluginService.isNetworkError(error); + if (this.pluginService.isNetworkError(error)) { + return true; + } + // A demoted writer returns read-only errors on writes, which must trigger failover. + if (this.failoverMode === FailoverMode.STRICT_WRITER) { + return this.pluginService.isReadOnlyConnectionError(error); + } } return false; diff --git a/common/lib/plugins/failover2/failover2_plugin.ts b/common/lib/plugins/failover2/failover2_plugin.ts index 5d7060d03..d5787ac07 100644 --- a/common/lib/plugins/failover2/failover2_plugin.ts +++ b/common/lib/plugins/failover2/failover2_plugin.ts @@ -475,7 +475,13 @@ export class Failover2Plugin extends AbstractConnectionPlugin implements CanRele } if (error instanceof Error) { - return this.pluginService.isNetworkError(error); + if (this.pluginService.isNetworkError(error)) { + return true; + } + // A demoted writer returns read-only errors on writes, which must trigger failover. + if (this.failoverMode === FailoverMode.STRICT_WRITER) { + return this.pluginService.isReadOnlyConnectionError(error); + } } return false; diff --git a/mysql/lib/mysql_error_handler.ts b/mysql/lib/mysql_error_handler.ts index 45066735c..1713339d4 100644 --- a/mysql/lib/mysql_error_handler.ts +++ b/mysql/lib/mysql_error_handler.ts @@ -24,6 +24,7 @@ export class MySQLErrorHandler implements ErrorHandler { private unexpectedError: Error | null = null; protected static readonly SYNTAX_ERROR_CODES = ["42000", "42S02"]; protected static readonly SYNTAX_ERROR_MESSAGE = "You have an error in your SQL syntax"; + protected static readonly READ_ONLY_ERROR_CODES = [1290, 1836]; protected isNoOpListenerAttached = false; protected isTrackingListenerAttached = false; @@ -69,6 +70,14 @@ export class MySQLErrorHandler implements ErrorHandler { return e.message.includes(MySQLErrorHandler.SYNTAX_ERROR_MESSAGE); } + isReadOnlyConnectionError(e: Error): boolean { + if (Object.prototype.hasOwnProperty.call(e, "errno")) { + // @ts-ignore + return MySQLErrorHandler.READ_ONLY_ERROR_CODES.includes(e["errno"]); + } + return false; + } + hasLoginError(): boolean { return this.unexpectedError !== null && this.isLoginError(this.unexpectedError); } diff --git a/pg/lib/abstract_pg_error_handler.ts b/pg/lib/abstract_pg_error_handler.ts index e5dcfa146..f13d63461 100644 --- a/pg/lib/abstract_pg_error_handler.ts +++ b/pg/lib/abstract_pg_error_handler.ts @@ -23,6 +23,7 @@ export abstract class AbstractPgErrorHandler implements ErrorHandler { protected unexpectedError: Error | null = null; protected static readonly SYNTAX_ERROR_CODE = "42601"; protected static readonly SYNTAX_ERROR_MESSAGE = "syntax error"; + protected static readonly READ_ONLY_CONNECTION_SQLSTATE = "25006"; protected isNoOpListenerAttached = false; protected isTrackingListenerAttached = false; @@ -72,6 +73,14 @@ export abstract class AbstractPgErrorHandler implements ErrorHandler { return e.message.includes(AbstractPgErrorHandler.SYNTAX_ERROR_MESSAGE); } + isReadOnlyConnectionError(e: Error): boolean { + if (Object.prototype.hasOwnProperty.call(e, "code")) { + // @ts-ignore + return AbstractPgErrorHandler.READ_ONLY_CONNECTION_SQLSTATE === e["code"]; + } + return false; + } + hasLoginError(): boolean { return this.unexpectedError !== null && this.isLoginError(this.unexpectedError); } diff --git a/tests/unit/error_handler.test.ts b/tests/unit/error_handler.test.ts new file mode 100644 index 000000000..12b57fc2f --- /dev/null +++ b/tests/unit/error_handler.test.ts @@ -0,0 +1,55 @@ +/* + Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + + Licensed under the Apache License, Version 2.0 (the "License"). + You may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +import { MySQLErrorHandler } from "../../mysql/lib/mysql_error_handler"; +import { PgErrorHandler } from "../../pg/lib/pg_error_handler"; + +function errorWith(props: Record): Error { + return Object.assign(new Error("test"), props); +} + +describe("test read only connection error", () => { + const pgHandler = new PgErrorHandler(); + const mysqlHandler = new MySQLErrorHandler(); + + it("test pg read only detected by sqlstate 25006", () => { + expect(pgHandler.isReadOnlyConnectionError(errorWith({ code: "25006" }))).toBe(true); + }); + + it("test pg unrelated sqlstate not detected", () => { + expect(pgHandler.isReadOnlyConnectionError(errorWith({ code: "42601" }))).toBe(false); + }); + + it("test pg error without code not detected", () => { + expect(pgHandler.isReadOnlyConnectionError(new Error("cannot execute in a read-only transaction"))).toBe(false); + }); + + it("test mysql read only detected by errno 1290", () => { + expect(mysqlHandler.isReadOnlyConnectionError(errorWith({ errno: 1290 }))).toBe(true); + }); + + it("test mysql read only detected by errno 1836", () => { + expect(mysqlHandler.isReadOnlyConnectionError(errorWith({ errno: 1836 }))).toBe(true); + }); + + it("test mysql unrelated errno not detected", () => { + expect(mysqlHandler.isReadOnlyConnectionError(errorWith({ errno: 1064 }))).toBe(false); + }); + + it("test mysql error without errno not detected", () => { + expect(mysqlHandler.isReadOnlyConnectionError(new Error("read only"))).toBe(false); + }); +}); diff --git a/tests/unit/failover2_plugin.test.ts b/tests/unit/failover2_plugin.test.ts index b36a2baf3..15c308a54 100644 --- a/tests/unit/failover2_plugin.test.ts +++ b/tests/unit/failover2_plugin.test.ts @@ -325,4 +325,56 @@ describe("reader failover handler", () => { expect(count).toStrictEqual(2); verify(mockRdsHostListProvider.getRdsUrlType()).never(); }); + + it("test execute - read-only error triggers failover in strict-writer mode", async () => { + properties.set(WrapperProperties.ENABLE_CLUSTER_AWARE_FAILOVER.name, true); + when(mockPluginService.getAllHosts()).thenReturn([builder.withHost("hostA").build()]); + when(mockPluginService.getCurrentHostInfo()).thenReturn(null); + when(mockPluginService.isNetworkError(anything())).thenReturn(false); + when(mockPluginService.isReadOnlyConnectionError(anything())).thenReturn(true); + + initializePlugin(instance(mockPluginService)); + const spyPlugin: Failover2Plugin = spy(plugin); + when(spyPlugin.failover()).thenResolve(); + plugin.failoverMode = FailoverMode.STRICT_WRITER; + + const readOnlyError = new Error("read only"); + await expect(plugin.execute("query", () => Promise.reject(readOnlyError))).rejects.toBe(readOnlyError); + + verify(spyPlugin.failover()).once(); + }); + + it("test execute - read-only error does not trigger failover outside strict-writer mode", async () => { + properties.set(WrapperProperties.ENABLE_CLUSTER_AWARE_FAILOVER.name, true); + when(mockPluginService.getAllHosts()).thenReturn([builder.withHost("hostA").build()]); + when(mockPluginService.isNetworkError(anything())).thenReturn(false); + when(mockPluginService.isReadOnlyConnectionError(anything())).thenReturn(true); + + initializePlugin(instance(mockPluginService)); + const spyPlugin: Failover2Plugin = spy(plugin); + when(spyPlugin.failover()).thenResolve(); + plugin.failoverMode = FailoverMode.READER_OR_WRITER; + + const readOnlyError = new Error("read only"); + await expect(plugin.execute("query", () => Promise.reject(readOnlyError))).rejects.toBe(readOnlyError); + + verify(spyPlugin.failover()).never(); + }); + + it("test execute - network error triggers failover regardless of mode", async () => { + properties.set(WrapperProperties.ENABLE_CLUSTER_AWARE_FAILOVER.name, true); + when(mockPluginService.getAllHosts()).thenReturn([builder.withHost("hostA").build()]); + when(mockPluginService.getCurrentHostInfo()).thenReturn(null); + when(mockPluginService.isNetworkError(anything())).thenReturn(true); + + initializePlugin(instance(mockPluginService)); + const spyPlugin: Failover2Plugin = spy(plugin); + when(spyPlugin.failover()).thenResolve(); + plugin.failoverMode = FailoverMode.READER_OR_WRITER; + + const networkError = new Error("network"); + await expect(plugin.execute("query", () => Promise.reject(networkError))).rejects.toBe(networkError); + + verify(spyPlugin.failover()).once(); + }); }); diff --git a/tests/unit/failover_plugin.test.ts b/tests/unit/failover_plugin.test.ts index 0cbbe34d1..4e536c846 100644 --- a/tests/unit/failover_plugin.test.ts +++ b/tests/unit/failover_plugin.test.ts @@ -430,4 +430,56 @@ describe("reader failover handler", () => { expect(count).toStrictEqual(2); verify(mockRdsHostListProvider.getRdsUrlType()).never(); }); + + it("test execute - read-only error triggers failover in strict-writer mode", async () => { + properties.set(WrapperProperties.ENABLE_CLUSTER_AWARE_FAILOVER.name, true); + when(mockPluginService.getAllHosts()).thenReturn([builder.withHost("hostA").build()]); + when(mockPluginService.getCurrentHostInfo()).thenReturn(null); + when(mockPluginService.isNetworkError(anything())).thenReturn(false); + when(mockPluginService.isReadOnlyConnectionError(anything())).thenReturn(true); + + initializePlugin(instance(mockPluginService)); + const spyPlugin: FailoverPlugin = spy(plugin); + when(spyPlugin.pickNewConnection()).thenResolve(); + plugin.failoverMode = FailoverMode.STRICT_WRITER; + + const readOnlyError = new Error("read only"); + await expect(plugin.execute("query", () => Promise.reject(readOnlyError))).rejects.toBe(readOnlyError); + + verify(spyPlugin.pickNewConnection()).once(); + }); + + it("test execute - read-only error does not trigger failover outside strict-writer mode", async () => { + properties.set(WrapperProperties.ENABLE_CLUSTER_AWARE_FAILOVER.name, true); + when(mockPluginService.getAllHosts()).thenReturn([builder.withHost("hostA").build()]); + when(mockPluginService.isNetworkError(anything())).thenReturn(false); + when(mockPluginService.isReadOnlyConnectionError(anything())).thenReturn(true); + + initializePlugin(instance(mockPluginService)); + const spyPlugin: FailoverPlugin = spy(plugin); + when(spyPlugin.pickNewConnection()).thenResolve(); + plugin.failoverMode = FailoverMode.READER_OR_WRITER; + + const readOnlyError = new Error("read only"); + await expect(plugin.execute("query", () => Promise.reject(readOnlyError))).rejects.toBe(readOnlyError); + + verify(spyPlugin.pickNewConnection()).never(); + }); + + it("test execute - network error triggers failover regardless of mode", async () => { + properties.set(WrapperProperties.ENABLE_CLUSTER_AWARE_FAILOVER.name, true); + when(mockPluginService.getAllHosts()).thenReturn([builder.withHost("hostA").build()]); + when(mockPluginService.getCurrentHostInfo()).thenReturn(null); + when(mockPluginService.isNetworkError(anything())).thenReturn(true); + + initializePlugin(instance(mockPluginService)); + const spyPlugin: FailoverPlugin = spy(plugin); + when(spyPlugin.pickNewConnection()).thenResolve(); + plugin.failoverMode = FailoverMode.READER_OR_WRITER; + + const networkError = new Error("network"); + await expect(plugin.execute("query", () => Promise.reject(networkError))).rejects.toBe(networkError); + + verify(spyPlugin.pickNewConnection()).once(); + }); });