fix: evict stale ban/mute/gag cache when records are removed externally - #110
Open
Prefix wants to merge 3 commits into
Open
fix: evict stale ban/mute/gag cache when records are removed externally#110Prefix wants to merge 3 commits into
Prefix wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses stale in-memory admin-operation caches (ban/mute/gag) when operation records are removed externally (e.g., via a web panel against a shared DB), by periodically re-validating cached identities against storage and evicting entries that no longer have an active record.
Changes:
- Adds a background refresh loop in
AdminOperationEngineto periodically validate cached handler identities against storage and evict stale entries on the game thread. - Extends
IAdminOperationHandlerwithGetCachedIdentities()(default empty implementation) and implements it for Ban/Mute/Gag handlers. - Introduces
AdminOperationService.TryHasActiveAsyncreturningbool?to distinguish storage failure (null) from definitive absence (false) to avoid eviction during outages.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Sharp.Modules/AdminCommands/src/Services/Handlers/MuteHandler.cs | Exposes mute cache keys for periodic refresh. |
| Sharp.Modules/AdminCommands/src/Services/Handlers/GagHandler.cs | Exposes gag cache keys for periodic refresh. |
| Sharp.Modules/AdminCommands/src/Services/Handlers/BanHandler.cs | Exposes ban cache keys for periodic refresh. |
| Sharp.Modules/AdminCommands/src/Services/AdminOperationService.cs | Adds tri-state “has active” query for safe cache eviction. |
| Sharp.Modules/AdminCommands/src/Services/AdminOperationEngine.cs | Starts/stops refresh loop and performs periodic cache re-validation + eviction. |
| Sharp.Modules/AdminCommands/Shared/IAdminOperationHandler.cs | Adds GetCachedIdentities() with default empty implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+483
to
+490
| foreach (var (handler, steamId) in stale) | ||
| { | ||
| handler.OnRemoved(steamId, _bridge.ClientManager.GetGameClient(steamId)); | ||
|
|
||
| _logger.LogInformation("Evicted cached {Type} for {SteamId}: no active record in storage", | ||
| handler.Type, | ||
| steamId); | ||
| } |
Comment on lines
+68
to
+77
| try | ||
| { | ||
| return await _storage.HasActiveAsync(steamId, type).ConfigureAwait(false); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Failed to check active {Type} for {SteamId}", type, steamId); | ||
|
|
||
| return null; | ||
| } |
Contributor
Author
|
Addressed the review feedback (
|
Handler caches were only populated/invalidated by same-process commands or lazy TTL checks, so an unban/unmute/ungag written directly to a shared database (e.g. from a web panel) never reached other servers' in-memory caches until reconnect. Adds a periodic background refresh that re-validates cached identities against storage and evicts entries with no active record. Storage failures are distinguished from removals so a database outage never mass-evicts.
…ache re-check A freshly-applied ban/mute/gag is persisted to storage asynchronously, so the 60s background re-check could query before the write is visible (notably on a shared/replicated DB) and wrongly evict a punishment an admin just set. Stamp each cache entry with AddedAt and have GetCachedIdentities(grace) skip entries younger than a 60s grace window, so a new entry is only re-validated once its write has had time to settle.
…sh loop Review follow-ups on the cache re-check: - TOCTOU: a punishment re-applied by an admin between the storage check and the evict frame was dropped unconditionally. Re-validate on the game thread — only evict SteamIDs the currently-registered handler still reports as grace-eligible (a re-applied entry has a fresh AddedAt, so it is grace-excluded and skipped). - Handler-swap race (Copilot): a handler can be replaced/unregistered between the snapshot and the evict; guard with a ReferenceEquals check against the currently registered handler, and wrap each OnRemoved in try/catch so one throwing handler doesn't abort the batch. - Loop: OperationCanceledException catch now filters on the token so a stray OCE can't permanently kill the refresh loop. - Grace window widened to 2x the refresh interval for slow-write headroom.
Prefix
force-pushed
the
fix/admin-operation-cache-refresh
branch
from
July 20, 2026 15:09
140cac5 to
521abe2
Compare
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.
Problem
AdminCommands handler caches (
BanHandler._bans/_ipBans,MuteHandler._mutes,GagHandler._gags) are only invalidated by same-processms_unban/ms_unmute/ms_ungagor by lazy TTL checks on the player's next action. When several servers share one operation database and a record is removed externally (web panel unban writing directly to the DB), other servers keep rejecting the player from their stale in-memory cache until a module reload.Discussed with Nuko on Discord — agreed resolution: background periodic re-check of cached entries, extracted as a reusable helper, covering mutes & gags as well.
Change
AdminOperationEnginestarts a background loop (60s interval) inInit(), cancelled inShutdown():HasActiveAsync,OnRemovedpath.IAdminOperationHandler.GetCachedIdentities()— new member with a default empty implementation (non-breaking); external handlers can opt in to the refresh by overriding it. Ban/Mute/Gag handlers return their cache keys.AdminOperationService.TryHasActiveAsync— tri-state variant (bool?) so a storage failure is distinguishable from "no active record"; the refresh never evicts on a database outage (the existingHasActiveAsyncswallows exceptions tofalse, which would mass-unban on a transient DB error).Cost is one
HasActiveAsyncper cached entry per minute — the cached set is only currently-punished players, so negligible.Non-goals
OnClientPostAdminCheckload; this PR only fixes stale removals).Tested:
dotnet buildclean, 0 warnings.