Add dashboard in-memory cache#54
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an in-memory caching layer for dashboard data in SAMA.Web to reduce repeated query load, plus a Quartz job to periodically refresh cached entries and cache invalidation hooks in command services.
Changes:
- Introduced
DashboardCacheServicewith per-workspace/per-hours caches, eviction, and stampede protection. - Added
DashboardCacheRefreshJobscheduled via Quartz to evict stale entries and refresh active cache keys. - Updated dashboard page + command services/tests to use/invalidate the new cache; removed Razor runtime compilation package/config.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| SAMA.Web/Services/DashboardCacheService.cs | New in-memory cache for dashboard data/timeline/trends with eviction and per-key locks. |
| SAMA.Web/Services/DashboardCacheRefreshJob.cs | New Quartz job to evict stale entries and refresh cached keys. |
| SAMA.Web/Services/Commands/WorkspaceCommandService.cs | Invalidates dashboard cache on workspace update/delete. |
| SAMA.Web/Services/Commands/CheckCommandService.cs | Invalidates dashboard cache on check create/update/delete. |
| SAMA.Web/Services/Commands/AlertCommandService.cs | Invalidates dashboard cache on alert create/update/delete. |
| SAMA.Web/SAMA.Web.csproj | Removes Razor runtime compilation package reference. |
| SAMA.Web/Program.cs | Registers DashboardCacheService + schedules refresh job; simplifies Razor Pages registration. |
| SAMA.Web/Pages/Dashboard/Index.cshtml.cs | Switches dashboard data loading to DashboardCacheService. |
| SAMA.Tests.Unit/Web/Services/DashboardCacheServiceTests.cs | Adds unit tests for cache hit/miss, invalidation, stampede prevention. |
| SAMA.Tests.Unit/Web/Pages/Workspaces/EditModelTests.cs | Updates substitute ctor args for WorkspaceCommandService new dependency. |
| SAMA.Tests.Unit/Web/Pages/Workspaces/DeleteModelTests.cs | Updates substitute ctor args for WorkspaceCommandService new dependency. |
| SAMA.Tests.Unit/Web/Pages/Workspaces/CreateModelTests.cs | Updates substitute ctor args for WorkspaceCommandService new dependency. |
| SAMA.Tests.Unit/Web/Pages/Dashboard/IndexModelTests.cs | Updates tests to use pre-populated cache instead of direct query service mocks. |
| SAMA.Tests.Unit/Web/Pages/Checks/EditModelTests.cs | Updates substitute ctor args for CheckCommandService new dependency. |
| SAMA.Tests.Unit/Web/Pages/Checks/DeleteModelTests.cs | Updates substitute ctor args for CheckCommandService new dependency. |
| SAMA.Tests.Unit/Web/Pages/Checks/CreateModelTests.cs | Updates substitute ctor args for CheckCommandService new dependency. |
| SAMA.Tests.Unit/Web/Pages/Alerts/EditModelTests.cs | Updates substitute ctor args for AlertCommandService new dependency. |
| SAMA.Tests.Unit/Web/Pages/Alerts/DeleteModelTests.cs | Updates substitute ctor args for AlertCommandService new dependency. |
| SAMA.Tests.Unit/Web/Pages/Alerts/CreateModelTests.cs | Updates substitute ctor args for AlertCommandService new dependency. |
| SAMA.Tests.Integration/Web/Services/Commands/WorkspaceCommandServiceTests.cs | Updates integration test service construction for new cache dependency. |
| SAMA.Tests.Integration/Web/Services/Commands/CheckCommandServiceTests.cs | Updates integration test service construction for new cache dependency. |
| SAMA.Tests.Integration/Web/Services/Commands/AlertCommandServiceTests.cs | Updates integration test service construction for new cache dependency. |
| SAMA.Shared/SAMA.Shared.csproj | Adds project version metadata. |
| SAMA.Data/SAMA.Data.csproj | Adds project version metadata. |
Comment on lines
+208
to
+232
| public void InvalidateWorkspace(Guid workspaceId) | ||
| { | ||
| _workspaceCache.TryRemove(workspaceId, out _); | ||
| } | ||
|
|
||
| public void InvalidateAllForWorkspace(Guid workspaceId) | ||
| { | ||
| _workspaceCache.TryRemove(workspaceId, out _); | ||
|
|
||
| foreach (var key in _timelineCache.Keys) | ||
| { | ||
| if (key.WorkspaceId == workspaceId) | ||
| { | ||
| _timelineCache.TryRemove(key, out _); | ||
| } | ||
| } | ||
|
|
||
| foreach (var key in _trendsCache.Keys) | ||
| { | ||
| if (key.WorkspaceId == workspaceId) | ||
| { | ||
| _trendsCache.TryRemove(key, out _); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Cache entries are removed (Invalidate*/EvictStaleEntries/EnforceSizeLimit), but the per-key SemaphoreSlim instances in _workspaceLocks/_timelineLocks/_trendsLocks are never removed or disposed. In a long-running instance (many workspaces / hours combinations), these lock dictionaries can grow without bound even after cache eviction. Consider removing the corresponding lock when its cache entry is removed (and disposing the semaphore), or switching to a different per-key stampede strategy that doesn’t require a separate lock dictionary.
Comment on lines
+337
to
+347
| public TValue Data { get; set; } | ||
|
|
||
| public DateTimeOffset LastRefreshedAt { get; set; } | ||
|
|
||
| public DateTimeOffset LastAccessedAt { get; set; } | ||
|
|
||
| public CacheEntry(TValue data) | ||
| { | ||
| Data = data; | ||
| LastRefreshedAt = DateTimeOffset.UtcNow; | ||
| LastAccessedAt = DateTimeOffset.UtcNow; |
There was a problem hiding this comment.
CacheEntry.LastAccessedAt/LastRefreshedAt are DateTimeOffset (a multi-field struct) that are read/written from multiple threads without synchronization. Reads/writes of non-trivially-sized structs are not guaranteed to be atomic, which can lead to torn values during eviction/size enforcement. Consider storing timestamps as an atomic long (e.g., UnixTimeMilliseconds via Interlocked.Exchange) or making CacheEntry immutable and replacing the whole entry in the dictionary when updating timestamps.
Suggested change
| public TValue Data { get; set; } | |
| public DateTimeOffset LastRefreshedAt { get; set; } | |
| public DateTimeOffset LastAccessedAt { get; set; } | |
| public CacheEntry(TValue data) | |
| { | |
| Data = data; | |
| LastRefreshedAt = DateTimeOffset.UtcNow; | |
| LastAccessedAt = DateTimeOffset.UtcNow; | |
| private long _lastRefreshedAtUnixTimeMilliseconds; | |
| private long _lastAccessedAtUnixTimeMilliseconds; | |
| public TValue Data { get; set; } | |
| public DateTimeOffset LastRefreshedAt | |
| { | |
| get => DateTimeOffset.FromUnixTimeMilliseconds(System.Threading.Interlocked.Read(ref _lastRefreshedAtUnixTimeMilliseconds)); | |
| set => System.Threading.Interlocked.Exchange(ref _lastRefreshedAtUnixTimeMilliseconds, value.ToUnixTimeMilliseconds()); | |
| } | |
| public DateTimeOffset LastAccessedAt | |
| { | |
| get => DateTimeOffset.FromUnixTimeMilliseconds(System.Threading.Interlocked.Read(ref _lastAccessedAtUnixTimeMilliseconds)); | |
| set => System.Threading.Interlocked.Exchange(ref _lastAccessedAtUnixTimeMilliseconds, value.ToUnixTimeMilliseconds()); | |
| } | |
| public CacheEntry(TValue data) | |
| { | |
| Data = data; | |
| var now = DateTimeOffset.UtcNow; | |
| _lastRefreshedAtUnixTimeMilliseconds = now.ToUnixTimeMilliseconds(); | |
| _lastAccessedAtUnixTimeMilliseconds = now.ToUnixTimeMilliseconds(); |
Comment on lines
+13
to
+18
| _cacheService.EvictStaleEntries(); | ||
|
|
||
| var workspaceIds = _cacheService.GetCachedWorkspaceIds(); | ||
| var timelineKeys = _cacheService.GetCachedTimelineKeys(); | ||
| var trendsKeys = _cacheService.GetCachedTrendsKeys(); | ||
|
|
There was a problem hiding this comment.
The refresh job re-queries and refreshes every cached workspace/timeline/trends key on every run. With EvictionThreshold at 10 minutes and the trigger at 30 seconds, a single short-lived dashboard visit can cause up to ~10 minutes of periodic refresh DB load even if the dashboard is no longer being viewed. Consider refreshing only entries accessed recently (or skipping refreshes when LastAccessedAt hasn’t changed / LastRefreshedAt is already recent), so cache retention by access doesn’t translate into sustained background query churn.
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
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.