Skip to content

BuildRetryPolicy for reliable membase #1315

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Oceania2018 merged 2 commits into from
Mar 26, 2026
Merged

BuildRetryPolicy for reliable membase #1315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e53b801
BuildRetryPolicy for reliable membase
haiping-chen Mar 26, 2026
e2b2452
Merge branch 'SciSharp:master' into master
hchen2020 Mar 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Plugins/BotSharp.Plugin.Membase/BotSharp.Plugin.Membase.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Polly" />
<PackageReference Include="Refit" />
<PackageReference Include="Refit.HttpClientFactory" />
</ItemGroup>
Expand Down
60 changes: 44 additions & 16 deletions src/Plugins/BotSharp.Plugin.Membase/GraphDb/MembaseGraphDb.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,6 @@
using BotSharp.Abstraction.Graph;
using BotSharp.Abstraction.Graph.Models;
using BotSharp.Abstraction.Graph.Options;
using BotSharp.Abstraction.Models;
using BotSharp.Abstraction.Options;
using BotSharp.Abstraction.Utilities;
using BotSharp.Plugin.Membase.Interfaces;
using Microsoft.Extensions.Logging;
using System.Text.Json;
using Polly;
using Polly.Timeout;
using Refit;

namespace BotSharp.Plugin.Membase.GraphDb;

Expand All @@ -28,6 +22,33 @@ public MembaseGraphDb(

public string Provider => "membase";

private const int RetryCount = 3;

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);
}
Comment on lines +27 to +50
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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


public async Task<GraphQueryResult> ExecuteQueryAsync(string query, GraphQueryExecuteOptions? options = null)
{
if (string.IsNullOrEmpty(options?.GraphId))
Expand All @@ -36,14 +57,17 @@ public async Task<GraphQueryResult> ExecuteQueryAsync(string query, GraphQueryEx
}

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
Comment on lines 59 to +66
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

{
Query = query,
Parameters = args
}));

return new GraphQueryResult
{
Expand All @@ -52,10 +76,14 @@ public async Task<GraphQueryResult> ExecuteQueryAsync(string query, GraphQueryEx
Result = JsonSerializer.Serialize(response.Data)
};
}
catch (ApiException ex)
{
_logger.LogError($"Error when executing query in {Provider} graph db:\r\n{ex.Content}\r\n{query}\r\n{argLogs}");
throw;
Comment on lines +79 to +82
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

}
catch (Exception ex)
{
var argLogs = args.Select(x => (new KeyValue(x.Key, x.Value.ConvertToString(BotSharpOptions.defaultJsonOptions))).ToString());
_logger.LogError(ex, $"Error when executing query in {Provider} graph db. (Query: {query}), (Argments: \r\n{string.Join("\r\n", argLogs)})");
_logger.LogError(ex, $"Error when executing query in {Provider} graph db. (Query: {query}), (Argments: \r\n{argLogs})");
throw;
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/Plugins/BotSharp.Plugin.Membase/MembasePlugin.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ public void RegisterDI(IServiceCollection services, IConfiguration config)
.ConfigureHttpClient(c =>
{
c.BaseAddress = new Uri(settings.Host);
c.Timeout = TimeSpan.FromSeconds(settings.TimeoutSecond);
// Timeout is set by MembaseGrapbDb internally, but we set it here as well to ensure that the Refit client does not timeout before the graph db does.
c.Timeout = TimeSpan.FromSeconds(90);
});

services.AddScoped<IGraphDb, MembaseGraphDb>();
Expand Down
2 changes: 1 addition & 1 deletion src/Plugins/BotSharp.Plugin.Membase/Settings/MembaseSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ public class MembaseSettings
public string Host { get; set; } = "localhost";
public string ProjectId { get; set; } = string.Empty;
public string ApiKey { get; set; } = string.Empty;
public int TimeoutSecond { get; set; } = 10;
public int TimeoutSecond { get; set; } = 30;
public GraphInstance[] GraphInstances { get; set; } = [];
}
9 changes: 9 additions & 0 deletions src/Plugins/BotSharp.Plugin.Membase/Using.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,15 @@
global using System.Collections.Generic;
global using System.Linq;
global using System.Threading.Tasks;
global using System.Net;
global using System.Net.Http;
global using System.Text.Json;

global using Microsoft.AspNetCore.Authorization;
global using Microsoft.AspNetCore.Mvc;
global using Microsoft.Extensions.Configuration;
global using Microsoft.Extensions.DependencyInjection;
global using Microsoft.Extensions.Logging;

global using BotSharp.Abstraction.Users;
global using BotSharp.Abstraction.Knowledges;
Expand All @@ -15,3 +19,8 @@
global using BotSharp.Plugin.Membase.Models;
global using BotSharp.Plugin.Membase.Services;
global using BotSharp.Plugin.Membase.Settings;
global using BotSharp.Abstraction.Graph;
global using BotSharp.Abstraction.Graph.Models;
global using BotSharp.Abstraction.Graph.Options;
global using BotSharp.Abstraction.Options;
global using BotSharp.Plugin.Membase.Interfaces;
Loading