Skip to content

add timeout in membase #1307

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
iceljc merged 1 commit into from
Mar 12, 2026
Merged

add timeout in membase #1307

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 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 @@ -27,7 +27,11 @@ public void RegisterDI(IServiceCollection services, IConfiguration config)
CollectionFormat = CollectionFormat.Multi
})
.AddHttpMessageHandler<MembaseAuthHandler>()
.ConfigureHttpClient(c => c.BaseAddress = new Uri(settings.Host));
.ConfigureHttpClient(c =>
{
c.BaseAddress = new Uri(settings.Host);
Copy link
Contributor

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

Choose a reason for hiding this comment

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

Action required

1. Unvalidated settings.host uri 📘 Rule violation ⛯ Reliability

The new code constructs and assigns BaseAddress directly from settings.Host without
null/empty/format validation, which can throw during startup if config is missing or not an absolute
URI. This violates the requirement to add null/empty guards and safe fallbacks at external/config
boundaries.
Agent Prompt 
## Issue description
`settings.Host` is used at a configuration boundary without null/empty/format checks (`new Uri(settings.Host)`), which can throw at runtime (e.g., missing scheme, empty string).

## Issue Context
`MembaseSettings` is populated via `config.Bind("Membase", settings)`, so `Host` is externally provided and should be validated before using it as `HttpClient.BaseAddress`.

## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.Membase/MembasePlugin.cs[30-34]
- src/Plugins/BotSharp.Plugin.Membase/Settings/MembaseSettings.cs[3-8]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

c.Timeout = TimeSpan.FromSeconds(5);
Copy link
Contributor

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

Choose a reason for hiding this comment

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

Action required

2. Timeout masked as success 🐞 Bug ✓ Correctness

When IMembaseApi calls fail (including from the new 5s timeout), MembaseGraphDb catches the
exception and returns an empty GraphQueryResult, and MembaseController then returns HTTP 200 with
empty Columns/Items. This turns timeouts into silent, incorrect "successful" responses for
/membase/{graphId}/query.
Agent Prompt 
### Issue description
Failures from Membase API calls (including HttpClient timeouts introduced in this PR) are swallowed in `MembaseGraphDb.ExecuteQueryAsync` and returned as `new GraphQueryResult()`. `MembaseController` then always returns `200 OK` with empty `Columns/Items`, masking timeouts as successful responses.

### Issue Context
This becomes significantly more impactful after the PR because a 5-second HttpClient timeout will trigger cancellations in normal operation.

### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.Membase/GraphDb/MembaseGraphDb.cs[39-60]
- src/Plugins/BotSharp.Plugin.Membase/Controllers/MembaseController.cs[40-60]
- src/Plugins/BotSharp.Plugin.Membase/MembasePlugin.cs[30-34]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

});

services.AddScoped<IGraphDb, MembaseGraphDb>();

Expand Down
Loading