added retry logic to detect and recover from stale connections#29
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 22 minutes and 1 second. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughExecuteProcedure is refactored to retry executions on stale/lost Oracle connections using new options ChangesStale Connection Recovery and Retry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.Tests/UnitTests.cs (2)
737-739: ⚡ Quick winShared
_optionsmutation may cause test isolation issues.The test modifies the shared static
_optionsinstance without restoring original values. If tests run in parallel or a subsequent test assumes different default values, this could cause flaky failures.Consider using a local
Optionsinstance instead:Proposed fix
+ var options = new Options + { + ThrowErrorOnFailure = true, + TimeoutSeconds = 30, + BindParameterByName = true, + CloseConnection = false, + ClearConnectionPools = false + }; - _options.CloseConnection = false; - _options.ClearConnectionPools = false; - _options.ThrowErrorOnFailure = true; - var result1 = await Oracle.ExecuteProcedure(_input, output, _options, CancellationToken.None); + var result1 = await Oracle.ExecuteProcedure(_input, output, options, CancellationToken.None); ... - var result2 = await Oracle.ExecuteProcedure(_input, output, _options, CancellationToken.None); + var result2 = await Oracle.ExecuteProcedure(_input, output, options, CancellationToken.None);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.Tests/UnitTests.cs` around lines 737 - 739, The test mutates the shared static _options which can break test isolation; instead create and use a fresh local Options instance (e.g., var options = new Options { CloseConnection = false, ClearConnectionPools = false, ThrowErrorOnFailure = true }) for this test and pass that local instance into the method under test (or replace uses of _options within this test with the new local variable), or if you must reuse _options save its original property values and restore them in a finally/teardown block; specifically change references to _options.CloseConnection, _options.ClearConnectionPools and _options.ThrowErrorOnFailure in this test to operate on the local options variable (or ensure restoration) to avoid cross-test side effects.
746-760: 💤 Low valueTest leaves connection cached after completion—may affect other tests.
The test intentionally keeps
CloseConnection = falsebut doesn't clean up the cached connection afterward. TheTearDownmethod callsDropTestTableandDropProcedure, but the stale cached connection totest_userremains. If subsequent tests expect a fresh connection state, they may fail.Consider adding cleanup at the end:
Proposed cleanup
var result2 = await Oracle.ExecuteProcedure(_input, output, _options, CancellationToken.None); ClassicAssert.IsTrue(result2.Success); var dict2 = (Dictionary<string, object>)result2.Output; ClassicAssert.AreEqual("haapatie 9", dict2["address"]); + + // Cleanup: close cached connection for subsequent tests + var cleanupOptions = new Options + { + ThrowErrorOnFailure = false, + CloseConnection = true, + ClearConnectionPools = true + }; + await Oracle.ExecuteProcedure(_input, output, cleanupOptions, CancellationToken.None); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.Tests/UnitTests.cs` around lines 746 - 760, The test leaves a cached connection for the test_user because CloseConnection = false, so after creating the test user/table/procedure you must clear the pooled/stale connection to avoid affecting later tests; after the recreateCon block (or in TearDown alongside DropTestTable and DropProcedure) call the appropriate pool-clear routine (e.g., OracleConnection.ClearPool(recreateCon) or OracleConnection.ClearAllPools()) or explicitly close/dispose any cached connection for test_user so the cached connection is removed and later tests get a fresh connection state.Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.cs (1)
154-162: 💤 Low valuePotential infinite recursion if
GetLazyConnectionreturns a stale connection twice.When
IsStaleConnectionExceptionis caught, the code clears the cache and pools, then callsGetLazyConnectionagain and attemptsOpenAsync. If this secondOpenAsyncalso throws a stale connection exception, it will propagate unhandled, which is acceptable. However, there's a subtle issue: the newLazy<OracleConnection>fromGetLazyConnectionmight still reference a bad pool state ifClearAllPoolshasn't fully completed. Consider awaiting a brief delay or documenting this limitation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.cs` around lines 154 - 162, The catch for IsStaleConnectionException can recreate a Lazy<OracleConnection> via GetLazyConnection and immediately call con.OpenAsync which can fail again if pools aren't fully cleared; modify the logic in the catch block handling IsStaleConnectionException to perform a bounded retry: remove the stale cache entry (LazyConnectionCache.TryRemove), call OracleConnection.ClearAllPools(), then recreate the connection with GetLazyConnection and attempt OpenAsync with a small exponential backoff (e.g., delay 100–500ms) and a max retry count (e.g., 2–3 attempts) before letting the exception propagate; reference GetLazyConnection, LazyConnectionCache, OracleConnection.ClearAllPools, IsStaleConnectionException and con.OpenAsync when implementing the retry/delay to avoid repeating stale connections.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.Tests/Lib/Helpers.cs`:
- Around line 91-95: The PL/SQL procedure created in Helpers.cs uses the
parameter name "name" which is shadowed by the table column `name` in the query
`where name = name`; change the parameter or qualify the column so the
right-hand side refers to the parameter (for example rename the parameter to
`p_name` or prefix it, or use a table alias like `w.name = name`) and update the
procedure creation string in the method that builds `procedureName` accordingly;
also fix the same shadowing occurrences in the inline procedure definitions in
UnitTests.cs so tests actually filter by the input parameter rather than the
column.
In
`@Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.cs`:
- Around line 59-69: The IsFaulted check after calling
command.ExecuteNonQueryAsync(cancellationToken) is unreliable/dead code; remove
the entire if (runCommand.IsFaulted) { ... } block and let awaiting the
Task<int> (runCommand) propagate exceptions to the existing catch handler. Keep
the call to ExecuteNonQueryAsync and the await var rowsAffected = await
runCommand; and rely on the existing catch to handle errors and use
options.ThrowErrorOnFailure / new Result(...) there rather than inspecting
runCommand.IsFaulted.
- Around line 103-104: The current throws use ArgumentException("Error when
executing command:", ex.Message) which passes ex.Message as paramName and loses
the original exception; update both occurrences (the throw inside the
options.ThrowErrorOnFailure branch and the similar throw at the later location)
to construct the ArgumentException with the caught exception as the inner
exception (pass the Exception object, not ex.Message) so the original stack and
message are preserved (e.g., use the overload that accepts an inner exception),
keeping the descriptive message "Error when executing command" and the original
exception as innerException.
---
Nitpick comments:
In
`@Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.Tests/UnitTests.cs`:
- Around line 737-739: The test mutates the shared static _options which can
break test isolation; instead create and use a fresh local Options instance
(e.g., var options = new Options { CloseConnection = false, ClearConnectionPools
= false, ThrowErrorOnFailure = true }) for this test and pass that local
instance into the method under test (or replace uses of _options within this
test with the new local variable), or if you must reuse _options save its
original property values and restore them in a finally/teardown block;
specifically change references to _options.CloseConnection,
_options.ClearConnectionPools and _options.ThrowErrorOnFailure in this test to
operate on the local options variable (or ensure restoration) to avoid
cross-test side effects.
- Around line 746-760: The test leaves a cached connection for the test_user
because CloseConnection = false, so after creating the test user/table/procedure
you must clear the pooled/stale connection to avoid affecting later tests; after
the recreateCon block (or in TearDown alongside DropTestTable and DropProcedure)
call the appropriate pool-clear routine (e.g.,
OracleConnection.ClearPool(recreateCon) or OracleConnection.ClearAllPools()) or
explicitly close/dispose any cached connection for test_user so the cached
connection is removed and later tests get a fresh connection state.
In
`@Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.cs`:
- Around line 154-162: The catch for IsStaleConnectionException can recreate a
Lazy<OracleConnection> via GetLazyConnection and immediately call con.OpenAsync
which can fail again if pools aren't fully cleared; modify the logic in the
catch block handling IsStaleConnectionException to perform a bounded retry:
remove the stale cache entry (LazyConnectionCache.TryRemove), call
OracleConnection.ClearAllPools(), then recreate the connection with
GetLazyConnection and attempt OpenAsync with a small exponential backoff (e.g.,
delay 100–500ms) and a max retry count (e.g., 2–3 attempts) before letting the
exception propagate; reference GetLazyConnection, LazyConnectionCache,
OracleConnection.ClearAllPools, IsStaleConnectionException and con.OpenAsync
when implementing the retry/delay to avoid repeating stale connections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 845eab61-7a1d-4a7a-bf49-af50acf0032d
📒 Files selected for processing (5)
Frends.Oracle.ExecuteProcedure/CHANGELOG.mdFrends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.Tests/Lib/Helpers.csFrends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.Tests/UnitTests.csFrends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.csFrends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.csproj
| { | ||
| var con = GetLazyConnection(input.ConnectionString); | ||
| await using var command = new OracleCommand(); | ||
| const int maxAttempts = 2; |
Contributor
There was a problem hiding this comment.
move to to options, so user can set another value if he wants (describe in summary what case is handles by thsi retry). Additionally add delay (also customizable by user) between attempts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.cs (1)
307-317: 💤 Low valueUnused method overload.
This synchronous overload is never called—only the async version with
delayMs(lines 319-333) is invoked at line 95. Consider removing this dead code.🧹 Proposed removal
- [ExcludeFromCodeCoverage] - private static bool RetryOnStaleConnection(Exception ex, int attempt, int maxAttempts, string connectionString) - { - if (attempt < maxAttempts && IsStaleConnectionException(ex)) - { - LazyConnectionCache.TryRemove(connectionString, out _); - OracleConnection.ClearAllPools(); - return true; - } - return false; - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.cs` around lines 307 - 317, Remove the dead synchronous overload RetryOnStaleConnection(Exception ex, int attempt, int maxAttempts, string connectionString) because only the async overload with delayMs is ever used; delete this method and any internal references to LazyConnectionCache.TryRemove(...) and OracleConnection.ClearAllPools() within it, or consolidate its logic into the single async RetryOnStaleConnection(...) if you intended a unified implementation; after removal, rebuild and run tests to confirm no callers remain (search for RetryOnStaleConnection to verify).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.Tests/UnitTests.cs`:
- Around line 797-815: The test
ExecuteProcedure_WithInvalidSql_ThrowsExceptionWhenThrowErrorOnFailureTrue must
be made async and must await the Assert.ThrowsAsync call; change the method
signature to "public async Task
ExecuteProcedure_WithInvalidSql_ThrowsExceptionWhenThrowErrorOnFailureTrue()"
and await the assertion like "var ex = await Assert.ThrowsAsync<Exception>(async
() => await Oracle.ExecuteProcedure(_input, output, _options,
CancellationToken.None));" so the exception task is observed and the test
correctly fails on assertion failures.
---
Nitpick comments:
In
`@Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.cs`:
- Around line 307-317: Remove the dead synchronous overload
RetryOnStaleConnection(Exception ex, int attempt, int maxAttempts, string
connectionString) because only the async overload with delayMs is ever used;
delete this method and any internal references to
LazyConnectionCache.TryRemove(...) and OracleConnection.ClearAllPools() within
it, or consolidate its logic into the single async RetryOnStaleConnection(...)
if you intended a unified implementation; after removal, rebuild and run tests
to confirm no callers remain (search for RetryOnStaleConnection to verify).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fdd3ff7-f46c-4226-9a5c-6f3821e2f0f7
📒 Files selected for processing (5)
Frends.Oracle.ExecuteProcedure/CHANGELOG.mdFrends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.Tests/Lib/Helpers.csFrends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.Tests/UnitTests.csFrends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure/Definitions/Options.csFrends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.cs
✅ Files skipped from review due to trivial changes (1)
- Frends.Oracle.ExecuteProcedure/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.Tests/Lib/Helpers.cs
Comment on lines
+797
to
+815
| [Test] | ||
| public void ExecuteProcedure_WithInvalidSql_ThrowsExceptionWhenThrowErrorOnFailureTrue() | ||
| { | ||
| _input.Command = "INVALID SQL SYNTAX HERE"; | ||
| _input.CommandType = OracleCommandType.Command; | ||
|
|
||
| var output = new Output | ||
| { | ||
| DataReturnType = OracleCommandReturnType.AffectedRows | ||
| }; | ||
|
|
||
| _options.ThrowErrorOnFailure = true; | ||
|
|
||
| var ex = Assert.ThrowsAsync<Exception>(async () => | ||
| await Oracle.ExecuteProcedure(_input, output, _options, CancellationToken.None)); | ||
|
|
||
| ClassicAssert.IsNotNull(ex); | ||
| ClassicAssert.IsTrue(ex.Message.Contains("Error when executing command")); | ||
| } |
Contributor
There was a problem hiding this comment.
Assert.ThrowsAsync must be awaited; test method must be async.
Assert.ThrowsAsync<T> returns Task<T>, not T. Without awaiting, the test completes before the async operation finishes, potentially passing even when the assertion fails. The method must be async Task and the assertion must be awaited.
🐛 Proposed fix
[Test]
- public void ExecuteProcedure_WithInvalidSql_ThrowsExceptionWhenThrowErrorOnFailureTrue()
+ public async Task ExecuteProcedure_WithInvalidSql_ThrowsExceptionWhenThrowErrorOnFailureTrue()
{
_input.Command = "INVALID SQL SYNTAX HERE";
_input.CommandType = OracleCommandType.Command;
var output = new Output
{
DataReturnType = OracleCommandReturnType.AffectedRows
};
_options.ThrowErrorOnFailure = true;
- var ex = Assert.ThrowsAsync<Exception>(async () =>
+ var ex = await Assert.ThrowsAsync<Exception>(async () =>
await Oracle.ExecuteProcedure(_input, output, _options, CancellationToken.None));
ClassicAssert.IsNotNull(ex);
ClassicAssert.IsTrue(ex.Message.Contains("Error when executing command"));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Frends.Oracle.ExecuteProcedure/Frends.Oracle.ExecuteProcedure.Tests/UnitTests.cs`
around lines 797 - 815, The test
ExecuteProcedure_WithInvalidSql_ThrowsExceptionWhenThrowErrorOnFailureTrue must
be made async and must await the Assert.ThrowsAsync call; change the method
signature to "public async Task
ExecuteProcedure_WithInvalidSql_ThrowsExceptionWhenThrowErrorOnFailureTrue()"
and await the assertion like "var ex = await Assert.ThrowsAsync<Exception>(async
() => await Oracle.ExecuteProcedure(_input, output, _options,
CancellationToken.None));" so the exception task is observed and the test
correctly fails on assertion failures.
|
|
||
| /// <summary> | ||
| /// Maximum number of retry attempts when encountering stale Oracle connections after server restart. | ||
| /// Valid values: 1-5. Value of 1 means no retry. |
Contributor
There was a problem hiding this comment.
Change code logic then - As a user if I put 1 retry attempts I expect 1 retry attempt not 0 xd
MatteoDelOmbra
approved these changes
May 28, 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
2 participants
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.
Please review my changes :)
Review Checklist
Summary by CodeRabbit