diff --git a/src/Persistence/Oracle/OracleTests/Transport/clear_all_wolverine_storage.cs b/src/Persistence/Oracle/OracleTests/Transport/clear_all_wolverine_storage.cs index ca140792d..9f255d13d 100644 --- a/src/Persistence/Oracle/OracleTests/Transport/clear_all_wolverine_storage.cs +++ b/src/Persistence/Oracle/OracleTests/Transport/clear_all_wolverine_storage.cs @@ -1,5 +1,4 @@ using IntegrationTests; -using JasperFx.Core; using Oracle.ManagedDataAccess.Client; using Weasel.Oracle; using Wolverine; @@ -77,17 +76,21 @@ protected override async Task beforeHostAsync() /// 30835978599) and burned this class's two retries in the run after it. Left alone the count /// climbs monotonically — a local repro reached 3. /// - /// Now that the DROP resolves a table that actually exists, ORA-00054 becomes reachable - /// for the first time: this class is the only provider in the suite that attaches a listener, - /// and a polling listener holds a TM lock on the queue table (see the matching retry in - /// OracleQueue.TeardownAsync). Hence 942 = done, 54 = wait and retry, anything else - /// throws. Exhausting the retries throws too, rather than returning quietly: "could not drop the - /// queue table" is a far better failure than a count assertion two calls later that names - /// neither the lock nor the leftover rows. + /// Once the DROP started resolving a table that actually exists, ORA-00054 became + /// reachable for the first time, because this class was then the only provider in the suite + /// attaching a listener and a polling listener holds a TM lock on the queue table (see the + /// matching retry in OracleQueue.TeardownAsync). GH-3820 removed that listener — the + /// casing bug that forced it is fixed — so the contention should be gone rather than merely + /// retried around. The retry stays as a belt-and-braces guard: a host from a prior class in + /// this collection can still be tearing its connections down. Hence 942 = done, 54 = wait and + /// retry, anything else throws. Exhausting the retries throws too, rather than returning + /// quietly: "could not drop the queue table" is a far better failure than a count assertion two + /// calls later that names neither the lock nor the leftover rows. /// private static async Task dropTableWithRetryAsync(OracleConnection conn, string table) { const int maxAttempts = 5; + OracleException? lastBusy = null; for (var attempt = 1; attempt <= maxAttempts; attempt++) { try @@ -102,16 +105,26 @@ private static async Task dropTableWithRetryAsync(OracleConnection conn, string // ORA-00942: table or view does not exist — nothing to drop, which is the goal. return; } - catch (OracleException e) when (e.Number == 54 && attempt < maxAttempts) + catch (OracleException e) when (e.Number == 54) { // ORA-00054: resource busy. The prior host's connections are still tearing down. - await Task.Delay(TimeSpan.FromSeconds(2 * attempt)); + // + // Deliberately NOT guarded by `attempt < maxAttempts`. Folding the loop counter into + // the `when` clause meant that on the final attempt the guard was false, the raw + // OracleException escaped the loop, and the descriptive throw below was unreachable + // dead code -- it never ran once. GH-3820. + lastBusy = e; + if (attempt < maxAttempts) + { + await Task.Delay(TimeSpan.FromSeconds(2 * attempt)); + } } } throw new InvalidOperationException( $"Could not drop {TransportSchemaName}.{table} after {maxAttempts} attempts — it is still locked by " + - "another session. The test would otherwise start against a table holding a previous test's rows."); + "another session. The test would otherwise start against a table holding a previous test's rows.", + lastBusy); } protected override void ConfigureStorage(WolverineOptions options) @@ -119,12 +132,17 @@ protected override void ConfigureStorage(WolverineOptions options) options.PersistMessagesWithOracle(Servers.OracleConnectionString, SchemaName) .EnableMessageTransport(t => t.TransportSchemaName(TransportSchemaName)); - // Registered through the listener rather than as a subscriber, unlike the other - // providers: Oracle uppercases queue identifiers, but Uri.Host lowercases, so the - // publishing.To(queue.Uri) inside ToOracleQueue() resolves a *second* endpoint over the - // same physical tables. The polling interval keeps the listener from draining the queue - // out from under the assertions. - options.ListenToOracleQueue(QueueName).PollingInterval(1.Hours()); + // Subscriber only -- no listener, so nothing drains the queue out from under the + // assertions before the reset runs. + // + // This class used to attach a listener instead, because the publishing.To(queue.Uri) + // inside ToOracleQueue() resolved a *second* endpoint over the same physical tables: + // Oracle uppercases queue identifiers but Uri.Host lowercases, and + // OracleTransport.findEndpointByUri looked the name up without correcting it. That was a + // product bug, not a test quirk, and it is fixed in GH-3820 -- so this can now match every + // other provider in the suite. The listener was also the only thing holding a TM lock on + // the queue table, which is what made the ORA-00054 in beforeHostAsync reachable at all. + options.PublishAllMessages().ToOracleQueue(QueueName); } protected override ValueTask sendToQueueAsync(Envelope envelope) diff --git a/src/Persistence/Oracle/OracleTests/Transport/oracle_queue_uri_identity.cs b/src/Persistence/Oracle/OracleTests/Transport/oracle_queue_uri_identity.cs new file mode 100644 index 000000000..04b12bc7b --- /dev/null +++ b/src/Persistence/Oracle/OracleTests/Transport/oracle_queue_uri_identity.cs @@ -0,0 +1,45 @@ +using Shouldly; +using Wolverine.Oracle.Transport; +using Xunit; + +namespace OracleTests.Transport; + +/// +/// GH-3820. Oracle is the only database transport whose SanitizeIdentifier upper-cases +/// (Postgres and SQL Server both lower-case), and it inherited its siblings' findEndpointByUri +/// verbatim: Queues[uri.Host]. normalises the authority to lower +/// case, so an uppercased queue name never came back out of its own Uri — the lookup silently +/// created a *second* endpoint over the same physical queue tables. +/// +public class oracle_queue_uri_identity +{ + [Fact] + public void queue_uri_round_trips_to_the_same_endpoint() + { + var transport = new OracleTransport(); + var queue = transport.Queues[transport.MaybeCorrectName("resetone")]; + + // Oracle upper-cases identifiers... + queue.Name.ShouldBe("RESETONE"); + + // ...but System.Uri lower-cases the authority, so the Uri cannot carry the casing back. + queue.Uri.Host.ShouldBe("resetone"); + + // Resolving the endpoint from its own Uri must therefore correct the name again, or it + // hands back a brand new queue pointed at the same tables. + transport.GetOrCreateEndpoint(queue.Uri).ShouldBeSameAs(queue); + transport.Queues.Count().ShouldBe(1); + } + + [Fact] + public void queue_uri_round_trips_when_the_name_needs_dash_correction() + { + var transport = new OracleTransport(); + var queue = transport.Queues[transport.MaybeCorrectName("reset-two")]; + + queue.Name.ShouldBe("RESET_TWO"); + + transport.GetOrCreateEndpoint(queue.Uri).ShouldBeSameAs(queue); + transport.Queues.Count().ShouldBe(1); + } +} diff --git a/src/Persistence/Oracle/Wolverine.Oracle/Transport/OracleTransport.cs b/src/Persistence/Oracle/Wolverine.Oracle/Transport/OracleTransport.cs index 97839c6a0..744a249b4 100644 --- a/src/Persistence/Oracle/Wolverine.Oracle/Transport/OracleTransport.cs +++ b/src/Persistence/Oracle/Wolverine.Oracle/Transport/OracleTransport.cs @@ -46,7 +46,16 @@ public override string SanitizeIdentifier(string identifier) protected override OracleQueue findEndpointByUri(Uri uri) { - var queueName = uri.Host; + // GH-3820. Unlike the Postgres and SQL Server transports this was copied from, Oracle + // upper-cases identifiers (see SanitizeIdentifier) while System.Uri normalises the + // authority to lower case. A bare Queues[uri.Host] therefore never matches the key the + // queue was registered under, and LightweightCache quietly mints a *second* OracleQueue + // over the same physical tables. Correcting the name here is safe and idempotent: the + // host segment already went through SanitizeIdentifier when the Uri was built, so the + // only thing left to undo is Uri's lower-casing. Note SanitizeIdentifier rather than + // MaybeCorrectName -- the host already carries any IdentifierPrefix, which + // MaybeCorrectName would prepend a second time. + var queueName = SanitizeIdentifier(uri.Host); return Queues[queueName]; }