BuildRetryPolicy for reliable membase#1315
Merged
Oceania2018 merged 2 commits intoSciSharp:masterfrom Mar 26, 2026
Merged
Conversation
Contributor
Review Summary by QodoAdd retry policy with exponential backoff for Membase reliability
WalkthroughsDescription• Implement retry policy with exponential backoff for Membase API calls • Handle transient failures (HTTP 503, timeouts, task cancellations) • Increase default timeout from 10 to 30 seconds for reliability • Improve error logging with API exception details and serialized arguments • Add Polly dependency for resilient HTTP communication patterns Diagramflowchart LR
A["ExecuteQueryAsync"] --> B["BuildRetryPolicy"]
B --> C["Timeout Policy"]
B --> D["Retry Policy"]
D --> E["Handle Exceptions"]
E --> F["HttpRequestException"]
E --> G["TaskCanceledException"]
E --> H["TimeoutRejectedException"]
E --> I["ApiException 503"]
C --> J["Execute with Policy"]
D --> J
J --> K["CypherQueryAsync"]
File Changes1. src/Plugins/BotSharp.Plugin.Membase/GraphDb/MembaseGraphDb.cs
|
Contributor
Code Review by Qodo
1. Logs ex.Content and query
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+79
to
+82
| catch (ApiException ex) | ||
| { | ||
| _logger.LogError($"Error when executing query in {Provider} graph db:\r\n{ex.Content}\r\n{query}\r\n{argLogs}"); | ||
| throw; |
Contributor
There was a problem hiding this comment.
1. Logs ex.content and query 📘 Rule violation ⛨ Security
New error logging writes raw ex.Content, query, and serialized args (argLogs), which can include sensitive user/model content and internal details. This increases risk of sensitive data exposure through logs.
Agent Prompt
## Issue description
`MembaseGraphDb.ExecuteQueryAsync` logs raw `query`, serialized `args` (`argLogs`), and `ApiException.Content`, which may contain sensitive user/model content and/or internal configuration details.
## Issue Context
These values can contain user-provided text, identifiers, secrets, or verbose backend error payloads. Compliance requires minimizing and structuring logs to avoid sensitive content exposure.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.Membase/GraphDb/MembaseGraphDb.cs[60-60]
- src/Plugins/BotSharp.Plugin.Membase/GraphDb/MembaseGraphDb.cs[79-83]
- src/Plugins/BotSharp.Plugin.Membase/GraphDb/MembaseGraphDb.cs[86-86]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+27
to
+50
| private AsyncPolicy BuildRetryPolicy() | ||
| { | ||
| var settings = _services.GetRequiredService<MembaseSettings>(); | ||
| var timeoutSeconds = (double)settings.TimeoutSecond / RetryCount; | ||
|
|
||
| var timeoutPolicy = Policy.TimeoutAsync(TimeSpan.FromSeconds(timeoutSeconds)); | ||
|
|
||
| var retryPolicy = Policy | ||
| .Handle<HttpRequestException>() | ||
| .Or<TaskCanceledException>() | ||
| .Or<TimeoutRejectedException>() | ||
| .Or<ApiException>(ex => ex.StatusCode == HttpStatusCode.ServiceUnavailable) | ||
| .WaitAndRetryAsync( | ||
| retryCount: RetryCount, | ||
| sleepDurationProvider: retryAttempt => TimeSpan.FromSeconds(Math.Pow(2, retryAttempt)), | ||
| onRetry: (ex, timespan, retryAttempt, _) => | ||
| { | ||
| _logger.LogWarning(ex, | ||
| "CypherQueryAsync retry {RetryAttempt}/{MaxRetries} after {Delay}s. Exception: {Message}", | ||
| retryAttempt, RetryCount, timespan.TotalSeconds, ex.Message); | ||
| }); | ||
|
|
||
| return Policy.WrapAsync(retryPolicy, timeoutPolicy); | ||
| } |
Contributor
There was a problem hiding this comment.
2. Configured timeout not honored 🐞 Bug ⛯ Reliability
BuildRetryPolicy computes a per-attempt timeout as TimeoutSecond/RetryCount and applies it inside a retry policy with exponential backoff, so a single ExecuteQueryAsync can take significantly longer than TimeoutSecond. This makes TimeoutSecond misleading and can cause unexpectedly long requests under failure conditions.
Agent Prompt
## Issue description
`MembaseGraphDb.BuildRetryPolicy` derives a per-attempt timeout from `TimeoutSecond / RetryCount` and wraps it inside a retry policy with backoff, allowing total execution time to exceed `TimeoutSecond` and making the setting misleading.
## Issue Context
- Current wrap applies timeout per attempt, not to the whole operation.
- Backoff delays add additional wall time.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.Membase/GraphDb/MembaseGraphDb.cs[25-50]
## Suggested change
- Decide whether `TimeoutSecond` is **overall** timeout or **per-attempt** timeout.
- If it is overall: add an *outer* timeout policy for `TimeSpan.FromSeconds(settings.TimeoutSecond)` around the whole retry pipeline (and avoid dividing by retry count), or compute per-attempt timeout using `(RetryCount + 1)` and account for backoff.
- Consider renaming the constant to `MaxRetries` (retries) vs `MaxAttempts` (attempts) to avoid confusion.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
59
to
+66
| var args = options?.Arguments ?? new(); | ||
| var argLogs = JsonSerializer.Serialize(args, BotSharpOptions.defaultJsonOptions); | ||
|
|
||
| try | ||
| { | ||
| var response = await _membaseApi.CypherQueryAsync(options!.GraphId, new CypherQueryRequest | ||
| { | ||
| Query = query, | ||
| Parameters = args | ||
| }); | ||
| var retryPolicy = BuildRetryPolicy(); | ||
| var response = await retryPolicy.ExecuteAsync(() => | ||
| _membaseApi.CypherQueryAsync(options!.GraphId, new CypherQueryRequest |
Contributor
There was a problem hiding this comment.
3. Args serialization can break queries 🐞 Bug ✓ Correctness
ExecuteQueryAsync serializes options.Arguments before entering the try/catch, so a serialization failure will throw and prevent the Membase query from running. Since Arguments is Dictionary<string, object>, callers can provide values that are not JSON-serializable.
Agent Prompt
## Issue description
`ExecuteQueryAsync` computes `argLogs` via `JsonSerializer.Serialize` before the `try` block. If serialization fails, the method throws before executing `_membaseApi.CypherQueryAsync`, even though `argLogs` is only used for logging.
## Issue Context
`GraphQueryExecuteOptions.Arguments` is `Dictionary<string, object>` so values can be non-serializable.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.Membase/GraphDb/MembaseGraphDb.cs[59-61]
- src/Infrastructure/BotSharp.Abstraction/Graph/Options/GraphQueryOptions.cs[8-13]
## Suggested change
- Only serialize arguments inside the catch blocks (when you actually need to log), and wrap serialization in its own try/catch.
- Fallback to a safe placeholder (e.g., "<unserializable arguments>") if serialization fails.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
3 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.
No description provided.