Merged
Conversation
Replace SQLite VACUUM in scheduled cache cleanup with PRAGMA optimize, resolve DbContext instances per task execution, and log controller failures so blocked updates stop going silent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds a new unit test for search-cache cleanup; introduces a dedicated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 docstrings
🧪 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 |
Contributor
🔍 PR检查报告📋 检查概览
🧪 测试结果
📊 代码质量
📁 测试产物
🔗 相关链接此报告由GitHub Actions自动生成 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
TelegramSearchBot.Test/Service/Scheduler/SearchPageCacheCleanupTaskTests.cs (1)
24-38: Optional: registerDataDbContextas Scoped and drop the innerusingon scope-resolved contexts.
AddDbContextdefaults toServiceLifetime.Scopedfor a reason — the scope owns the context's lifetime and disposes it when the scope ends. Registering asTransientplus the manualusing var dbContext = scope.ServiceProvider.GetRequiredService<DataDbContext>();pattern causes a secondDispose()call (harmless on DbContext, but unusual) and diverges from how the production code resolves it. Switching to Scoped better mirrors real runtime wiring and simplifies the test.♻️ Optional refactor
- services.AddDbContext<DataDbContext>( - options => options.UseSqlite(_connection), - ServiceLifetime.Transient); + services.AddDbContext<DataDbContext>(options => options.UseSqlite(_connection)); ... - using var scope = _serviceProvider.CreateScope(); - using var dbContext = scope.ServiceProvider.GetRequiredService<DataDbContext>(); - dbContext.Database.EnsureCreated(); + using var scope = _serviceProvider.CreateScope(); + var dbContext = scope.ServiceProvider.GetRequiredService<DataDbContext>(); + dbContext.Database.EnsureCreated();Apply the same
using var scope/var dbContextshape to the seed block (lines 37–52) and the verification block (lines 65–69).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot.Test/Service/Scheduler/SearchPageCacheCleanupTaskTests.cs` around lines 24 - 38, Change the test DI registration to use the normal scoped lifetime instead of transient by updating the AddDbContext call (remove ServiceLifetime.Transient or replace with ServiceLifetime.Scoped) so DataDbContext is registered as scoped; then, when resolving from _serviceProvider.CreateScope(), stop wrapping the resolved DataDbContext in an extra using (replace "using var dbContext = scope.ServiceProvider.GetRequiredService<DataDbContext>()" with "var dbContext = scope.ServiceProvider.GetRequiredService<DataDbContext>()") so the scope owns disposal and the test mirrors production wiring (apply the same using-scope / var dbContext pattern to the seed and verification blocks that use DataDbContext).TelegramSearchBot/Service/Scheduler/WordCloudTask.cs (1)
141-154: Nice improvement — N+1 user lookup replaced with a single batched query.Correctness looks good:
ToDictionaryAsyncusesFuncselectors so the name-formatting runs client-side on materialized results, no translation risk. One small nit: the$"{u.FirstName} {u.LastName}".Trim()expression is built twice per row; extracting it once is slightly cleaner and avoids a second allocation.♻️ Optional refactor
- var userNames = await dbContext.UserData - .Where(u => topUserIds.Contains(u.Id)) - .ToDictionaryAsync( - u => u.Id, - u => string.IsNullOrWhiteSpace($"{u.FirstName} {u.LastName}".Trim()) - ? $"用户{u.Id}" - : $"{u.FirstName} {u.LastName}".Trim()); + var userNames = await dbContext.UserData + .Where(u => topUserIds.Contains(u.Id)) + .ToDictionaryAsync( + u => u.Id, + u => { + var fullName = $"{u.FirstName} {u.LastName}".Trim(); + return string.IsNullOrWhiteSpace(fullName) ? $"用户{u.Id}" : fullName; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Scheduler/WordCloudTask.cs` around lines 141 - 154, The ToDictionaryAsync selector in WordCloudTask.cs currently builds $"{u.FirstName} {u.LastName}".Trim() twice per row; change the selector passed to ToDictionaryAsync (used to populate userNames) to compute the trimmed fullName once into a local variable (e.g., fullName = $"{u.FirstName} {u.LastName}".Trim()) and then use string.IsNullOrWhiteSpace(fullName) ? $"用户{u.Id}" : fullName so the allocation and trimming happen only once; leave the rest of the topUsers projection (which uses userNames and topUserContributors) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@TelegramSearchBot.Test/Service/Scheduler/SearchPageCacheCleanupTaskTests.cs`:
- Around line 24-38: Change the test DI registration to use the normal scoped
lifetime instead of transient by updating the AddDbContext call (remove
ServiceLifetime.Transient or replace with ServiceLifetime.Scoped) so
DataDbContext is registered as scoped; then, when resolving from
_serviceProvider.CreateScope(), stop wrapping the resolved DataDbContext in an
extra using (replace "using var dbContext =
scope.ServiceProvider.GetRequiredService<DataDbContext>()" with "var dbContext =
scope.ServiceProvider.GetRequiredService<DataDbContext>()") so the scope owns
disposal and the test mirrors production wiring (apply the same using-scope /
var dbContext pattern to the seed and verification blocks that use
DataDbContext).
In `@TelegramSearchBot/Service/Scheduler/WordCloudTask.cs`:
- Around line 141-154: The ToDictionaryAsync selector in WordCloudTask.cs
currently builds $"{u.FirstName} {u.LastName}".Trim() twice per row; change the
selector passed to ToDictionaryAsync (used to populate userNames) to compute the
trimmed fullName once into a local variable (e.g., fullName = $"{u.FirstName}
{u.LastName}".Trim()) and then use string.IsNullOrWhiteSpace(fullName) ?
$"用户{u.Id}" : fullName so the allocation and trimming happen only once; leave
the rest of the topUsers projection (which uses userNames and
topUserContributors) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b09cb8d8-d177-41aa-b6d1-64d67c26dc66
📒 Files selected for processing (4)
TelegramSearchBot.Test/Service/Scheduler/SearchPageCacheCleanupTaskTests.csTelegramSearchBot/Executor/ControllerExecutor.csTelegramSearchBot/Service/Scheduler/SearchPageCacheCleanupTask.csTelegramSearchBot/Service/Scheduler/WordCloudTask.cs
Move search pagination cache traffic into a dedicated SearchCache.sqlite database, initialize it at startup, and route cache writes/cleanup through the isolated context so the primary SQLite file is no longer contended by cache maintenance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TelegramSearchBot/Service/Scheduler/SearchPageCacheCleanupTask.cs (1)
11-12: 🛠️ Refactor suggestion | 🟠 MajorUse the required singleton lifetime for scheduled tasks.
This task implements
IScheduledTask, but the injectable lifetime is stillTransient. The constructor now only capturesIServiceProviderandILogger, and the DbContext is resolved insideExecuteAsync(), so switching to singleton should not capture scoped state.Suggested change
- [Injectable(Microsoft.Extensions.DependencyInjection.ServiceLifetime.Transient)] + [Injectable(Microsoft.Extensions.DependencyInjection.ServiceLifetime.Singleton)] public class SearchPageCacheCleanupTask : IScheduledTask {As per coding guidelines,
**/Service/Scheduler/**/*.cs: “All scheduled tasks must implement IScheduledTask and use [Injectable(ServiceLifetime.Singleton)] attribute”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Scheduler/SearchPageCacheCleanupTask.cs` around lines 11 - 12, The SearchPageCacheCleanupTask is registered as Transient but must be a singleton per guidelines; update the Injectable attribute on the SearchPageCacheCleanupTask class (which implements IScheduledTask) to use Microsoft.Extensions.DependencyInjection.ServiceLifetime.Singleton instead of Transient, keeping the constructor that accepts IServiceProvider and ILogger as-is since DbContext is resolved inside ExecuteAsync().
🧹 Nitpick comments (1)
TelegramSearchBot.Database/Model/SearchCacheDbContext.cs (1)
17-19: Add an index for cleanup scans.Cleanup deletes filter by
CreatedTime, but this model only indexesUUID. Add a non-uniqueCreatedTimeindex so cache cleanup does not full-scan a high-churn table and hold the cache DB write lock longer than needed.Suggested change
modelBuilder.Entity<SearchPageCache>() .HasIndex(cache => cache.UUID) .IsUnique(); + + modelBuilder.Entity<SearchPageCache>() + .HasIndex(cache => cache.CreatedTime);Based on learnings, search results caching should be implemented via
SearchPageCacheCleanupTask.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot.Database/Model/SearchCacheDbContext.cs` around lines 17 - 19, The SearchPageCache entity currently only has a unique index on UUID which causes cleanup scans that filter by CreatedTime to full-scan the table; add a non-unique index on CreatedTime in the EF model configuration (in the same block where modelBuilder.Entity<SearchPageCache>() is configured) by adding a HasIndex(cache => cache.CreatedTime) without IsUnique(), so the cleanup task (SearchPageCacheCleanupTask) can efficiently delete by CreatedTime without long DB write-lock full scans.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@TelegramSearchBot/Service/Scheduler/SearchPageCacheCleanupTask.cs`:
- Around line 11-12: The SearchPageCacheCleanupTask is registered as Transient
but must be a singleton per guidelines; update the Injectable attribute on the
SearchPageCacheCleanupTask class (which implements IScheduledTask) to use
Microsoft.Extensions.DependencyInjection.ServiceLifetime.Singleton instead of
Transient, keeping the constructor that accepts IServiceProvider and ILogger
as-is since DbContext is resolved inside ExecuteAsync().
---
Nitpick comments:
In `@TelegramSearchBot.Database/Model/SearchCacheDbContext.cs`:
- Around line 17-19: The SearchPageCache entity currently only has a unique
index on UUID which causes cleanup scans that filter by CreatedTime to full-scan
the table; add a non-unique index on CreatedTime in the EF model configuration
(in the same block where modelBuilder.Entity<SearchPageCache>() is configured)
by adding a HasIndex(cache => cache.CreatedTime) without IsUnique(), so the
cleanup task (SearchPageCacheCleanupTask) can efficiently delete by CreatedTime
without long DB write-lock full scans.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8604a159-c20a-48a2-905c-a6d39d6f66a0
📒 Files selected for processing (8)
TelegramSearchBot.Database/Model/SearchCacheDbContext.csTelegramSearchBot.Test/Service/BotAPI/SendServiceTests.csTelegramSearchBot.Test/Service/Scheduler/SearchPageCacheCleanupTaskTests.csTelegramSearchBot/AppBootstrap/GeneralBootstrap.csTelegramSearchBot/Extension/ServiceCollectionExtension.csTelegramSearchBot/Service/BotAPI/SendService.csTelegramSearchBot/Service/Scheduler/SearchPageCacheCleanupTask.csTelegramSearchBot/Service/Search/SearchOptionStorageService.cs
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
1 participant
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.
Summary
n- initialize the isolated cache database on startup and route cache writes/cleanup through SearchCacheDbContextn- keep the primary Data.sqlite focused on durable bot data so cache churn no longer competes with message ingestionWhy
SQLite serializes writers per database file. The original problem was not just multiple DbContext instances, but that cache cleanup and normal message persistence were contending on the same Data.sqlite file. By splitting disposable search-page cache data into its own SQLite file, cache writes and cleanup no longer block the main message-processing database.
Validation
Summary by CodeRabbit
Bug Fixes
Chores