Repository navigation
GH-4482 follow-up: stop the Oracle poller swallowing failures, restore DLQ replay coverage - #4577
Merged
Merged
Conversation
…e DLQ replay coverage Two findings from the #4558 review. ### The Oracle external table poller swallowed every failure OracleMessageStore.PollForMessagesFromExternalTablesAsync caught Exception and wrote ex.Message to Console.Error. ExternalMessageTableListener already wraps that call and logs the failure through its ILogger with the table name, then keeps polling -- so catching here pre-empted the only structured report there was. An Oracle poll failure produced a bare line on stderr with no stack trace and nothing whatsoever in the configured log sink. It also printed on every shutdown cancellation. MessageDatabase<T>'s implementation does not catch, and this was the only Console.Error.WriteLine under src/Persistence. The catch is gone; the try/finally that releases the advisory lock stays, since that part IS Oracle-specific (OracleAdvisoryLock holds its own connection rather than riding the poll connection). ReleaseLockAsync swallows and logs its own errors and is time-bounded, so the finally cannot mask the exception on its way out. Guarded by a new Oracle test that points a listener at a table that does not exist, with AllowWolverineControl off so Wolverine cannot create it, and asserts the failure reaches the logger. Reinstating the catch turns it red. ### DLQ replay coverage came back, on all five providers pull_in_message_that_goes_to_dead_letter_queue_and_replay_it was deleted from the Postgres tests in #4558 rather than migrated, leaving nothing covering an external-table-sourced message reaching the dead letter queue and being replayed out of it. That path is not generic DLQ behaviour: ExternalMessageTableListener returns no-op CompleteAsync/DeferAsync and forces ShouldPersistBeforeProcessing to false. It now lives in ExternalTableTransportCompliance, so it covers PostgreSQL, Sql Server, MySql, SQLite and Oracle instead of Postgres alone. Two changes from the original: the wait for the message to reach the DLQ is bounded rather than an unbounded `while (!ids.Any())` spin, so "it never got dead lettered" fails instead of hanging; and the handler's completion source uses TrySetResult so a redelivery cannot tear the run down from a background thread. Verified: pinned `wolverine.slnx -c Release -f net9.0` clean (0 warnings), the external_message_tables suite green on all five providers (7 tests each), and the full OracleTests.Transport namespace green (33 tests). Both new tests were negative-controlled -- each fails when its fix is reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 24, 2026
Merged
This was referenced Sep 24, 2026
This was referenced Oct 1, 2026
This was referenced Oct 8, 2026
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.
The two substantive findings from the #4558 review. Independent of #4574 (the small cleanups), which can merge in either order.
1. The Oracle external table poller swallowed every failure
OracleMessageStore.PollForMessagesFromExternalTablesAsynccaughtExceptionand wroteex.MessagetoConsole.Error.ExternalMessageTableListeneralready wraps that call, logs the failure through itsILoggerwith the table name, and keeps polling. Catching inside the poller pre-empted the only structured report there was, so an Oracle poll failure produced a bare line on stderr — no stack trace, nothing in the configured log sink. It also printed on every shutdown cancellation.MessageDatabase<T>'s implementation deliberately does not catch, and this was the onlyConsole.Error.WriteLineanywhere undersrc/Persistence.The
catchis gone. Thetry/finallythat releases the advisory lock stays — that part genuinely is Oracle-specific, becauseOracleAdvisoryLockholds its own dedicated connection rather than riding the poll connection, so the lock must be released explicitly or no other node can take over. I checked thatReleaseLockAsyncswallows and logs its own errors and is time-bounded, so thefinallycannot mask the exception on its way out.Guarded by a test.
external_table_poll_failures_are_loggedpoints a listener at a table that does not exist withAllowWolverineControl = falseso Wolverine cannot create it, and asserts the failure reaches the logger.2. DLQ replay coverage is back, on all five providers
pull_in_message_that_goes_to_dead_letter_queue_and_replay_itwas deleted from the Postgres tests in #4558 rather than migrated. Nothing was left covering an external-table-sourced message reaching the dead letter queue and being replayed out of it — and that path is not generic DLQ behavior, sinceExternalMessageTableListenerreturns no-opCompleteAsync/DeferAsyncand forcesShouldPersistBeforeProcessingto false.It now lives in
ExternalTableTransportCompliance, so it covers PostgreSQL, Sql Server, MySql, SQLite and Oracle instead of Postgres alone. Four of those five never had this coverage. Two deliberate changes from the original:while (!ids.Any()), which turns "it never got dead lettered" into a hung run rather than a failure — bad in a suite that now runs on five providers.TrySetResult, so a redelivery after the waiter has completed cannot tear the run down with anInvalidOperationExceptionfrom a background thread.Verification
dotnet build wolverine.slnx -c Release -f net9.0— clean, 0 warnings, 0 errors. Re-run after rebasing onto currentmain.external_message_tablesgreen on all five providers, 7 tests each.OracleTests.Transportnamespace green, 33 tests.Both new tests were negative-controlled — each was confirmed to fail when its fix is reverted:
catchturnsexternal_table_poll_failures_are_loggedred, which also confirms the original defect was real rather than theoretical.MarkDeadLetterEnvelopesAsReplayableAsynccall turns the DLQ test red, so it is not vacuously green.Still open from the #4558 review
deleteManyAsyncmatching zero rows would still go green while the row redelivers on every poll.SendMessageThroughExternalTablenull-checks the result ofAs<IExternalDbTransportStore>(), which is a hard cast — the friendly message is unreachable.UseOraclePersistenceAndTransport, which does not exist (Oracle hasPersistMessagesWithOracle).external-tables.mdwas hand-edited rather than regenerated; the snippet footer still points at#L233-L295.🤖 Generated with Claude Code