feat: add timer trigger to refresh org cache#59
Conversation
WalkthroughUpdates project targets to .NET 10, adds a Redis-driven CacheRefreshTimer function and Redis singleton registration, and introduces a skipCache parameter to BrregService methods to allow bypassing cache reads. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/Dan.Plugin.Tilda/Functions/Timers.cs`:
- Around line 82-91: The logic in Timers.cs incorrectly checks mainKeyPrefix
before subunitKeyPrefix causing subunit keys to be misclassified; update the
conditional in the loop so that key.StartsWith(subunitKeyPrefix) is evaluated
before key.StartsWith(mainKeyPrefix) (i.e., check the more-specific
subunitKeyPrefix first) so that keys are stripped and added to subunitKeys (and
mainunitKeys) correctly; adjust the existing branches that populate mainunitKeys
and subunitKeys (using mainKeyPrefix, subunitKeyPrefix, mainunitKeys,
subunitKeys) without changing their stripping logic.
- Line 75: Replace the blocking call db.KeyExpireTime(key) inside the await
foreach loop with the async variant (await db.KeyExpireTimeAsync(key)) so the
loop does not block the thread; update the surrounding method to await the call
and handle the returned Task/TimeSpan appropriately (keep the await foreach
context and the variable name, e.g., expiry, but assign it from await
db.KeyExpireTimeAsync(key)).
- Around line 94-105: The current code in Timers.cs collects all calls to
brregService.GetFromBr for mainunitKeys and subunitKeys into a tasks list and
awaits Task.WhenAll, which can produce unbounded concurrent HTTP requests; fix
it by limiting concurrency—replace the generate-all-tasks pattern with a bounded
executor (e.g., use Parallel.ForEachAsync with a configurable
MaxDegreeOfParallelism or use a SemaphoreSlim around each call to
brregService.GetFromBr) and process keys from mainunitKeys and subunitKeys
through that bounded loop so only N requests run concurrently; ensure to await
completion of all operations and preserve includeSubunits:true/false per key
when invoking GetFromBr.
🧹 Nitpick comments (3)
src/Dan.Plugin.Tilda/Program.cs (1)
50-54: Local branch creates a separateConnectionMultiplexerfrom the one used by the Redis cache.In the Azure branch (lines 63–67), a single
IConnectionMultiplexeris created and shared with both the DI container andAddStackExchangeRedisCacheviaTask.FromResult. Here in the local branch,AddStackExchangeRedisCacheinternally creates its ownConnectionMultiplexerfrom the connection string, while line 54 registers a second one. This means two connections to Redis in the local branch vs. one in the Azure branch.Consider aligning them for consistency:
♻️ Suggested fix
- services.AddStackExchangeRedisCache(option => - { - option.Configuration = settings.RedisConnectionString; - }); - services.AddSingleton<IConnectionMultiplexer>(ConnectionMultiplexer.Connect(settings.RedisConnectionString)); + IConnectionMultiplexer localMultiplexer = ConnectionMultiplexer.Connect(settings.RedisConnectionString); + services.AddSingleton(localMultiplexer); + services.AddStackExchangeRedisCache(option => + { + option.ConnectionMultiplexerFactory = () => Task.FromResult(localMultiplexer); + });src/Dan.Plugin.Tilda/Services/BrregService.cs (1)
319-322: Pre-existing:DeserializeObjectcan returnnull, causingNullReferenceExceptiononAddRange.
JsonConvert.DeserializeObject<List<BREntityRegisterEntry>>(rawResult)returnsnullwhenrawResultis"null". Callingresult.AddRange(null)throwsArgumentNullException. Same pattern exists inGetOrganizationInfoFromBr(line 259 could returnnull). Not introduced by this PR, but worth a null-check since the timer will now exercise this path more frequently.🛡️ Suggested fix
var parsed = JsonConvert.DeserializeObject<List<BREntityRegisterEntry>>(rawResult); - result.AddRange(parsed); + if (parsed is not null) + { + result.AddRange(parsed); + }src/Dan.Plugin.Tilda/Functions/Timers.cs (1)
59-66: Consider adding error handling / logging around the timer body.If
GetFromBrthrows for one organization (e.g.,EvidenceSourcePermanentClientExceptionfor a not-found org), theTask.WhenAllwill propagate the exception and potentially abort the entire refresh cycle. Consider wrapping individual fetches in a try/catch so that one failure doesn't prevent the rest from being refreshed, and injectILoggerfor observability.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/Dan.Plugin.Tilda/Functions/Timers.cs`:
- Around line 75-80: The code in Timers.cs using db.KeyExpireTime(key) can
return null and currently results in silently skipping keys (expiry, timeLeft,
now); update the logic in the timer loop (where expiry, timeLeft are computed)
to explicitly check for a null expiry before doing the TimeSpan comparison, and
when expiry is null emit a debug/warn log (using the same logger used in this
class) indicating the key has no TTL so it can be inspected; optionally treat
null as requiring refresh (i.e., do not continue) if that matches intended
behavior.
- Around line 59-66: The CacheRefreshTimer function currently has no
observability; add structured logging using var logger =
context.GetLogger<Timers>() at the start of CacheRefreshTimer (log invocation
start and timestamp), after you enumerate/filter keys from
connectionMultiplexer.GetDatabase() (log counts for main-unit vs sub-unit keys),
after dispatching refresh tasks (log number of tasks dispatched), and a final
completion summary with success/failure counts and any caught exception details
(use logger.LogInformation and logger.LogError with placeholders for counts and
exception). Ensure logs reference the TimerTrigger invocation via
FunctionContext to correlate runs and include enough context (key counts,
durations, and error messages) to diagnose failures.
- Around line 94-105: The current Task.WhenAll(...) will throw an
AggregateException if any brregService.GetFromBr call fails and you lose which
keys succeeded; change to wrap each call in a safe task wrapper (e.g., create a
helper like SafeGetFromBrAsync(key, includeSubunits) or inline Task.Run with
try/catch) that calls brregService.GetFromBr inside a try block, catches/logs
the exception (including the failing key), and returns a sentinel (null or
Result object with success/failure) so all tasks complete without throwing; then
await Task.WhenAll on these wrapped tasks and handle/inspect each returned
result to determine which keys succeeded or failed.
| [Function("CacheRefreshTimer")] | ||
| public async Task CacheRefreshTimer( | ||
| //[HttpTrigger(AuthorizationLevel.Function, "get", "post")] HttpRequestData req, // easier to test locally using http trigger | ||
| [TimerTrigger("%CacheRefreshCron%")] TimerInfo timerInfo, | ||
| FunctionContext context) | ||
| { | ||
| var db = connectionMultiplexer.GetDatabase(); | ||
| var now = DateTime.UtcNow; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
No logging or observability in this timer function.
The function has zero log statements. For a background timer that enumerates Redis keys and makes external HTTP calls, this makes it very difficult to diagnose issues in production. At minimum, log:
- Timer invocation start
- Number of keys found / filtered
- Number of refresh tasks dispatched
- Completion summary (success/failure counts)
Example:
var logger = context.GetLogger<Timers>();
logger.LogInformation("CacheRefreshTimer started");
// ... after filtering ...
logger.LogInformation("Found {MainCount} main-unit and {SubCount} sub-unit keys to refresh", mainunitKeys.Count, subunitKeys.Count);🤖 Prompt for AI Agents
In `@src/Dan.Plugin.Tilda/Functions/Timers.cs` around lines 59 - 66, The
CacheRefreshTimer function currently has no observability; add structured
logging using var logger = context.GetLogger<Timers>() at the start of
CacheRefreshTimer (log invocation start and timestamp), after you
enumerate/filter keys from connectionMultiplexer.GetDatabase() (log counts for
main-unit vs sub-unit keys), after dispatching refresh tasks (log number of
tasks dispatched), and a final completion summary with success/failure counts
and any caught exception details (use logger.LogInformation and logger.LogError
with placeholders for counts and exception). Ensure logs reference the
TimerTrigger invocation via FunctionContext to correlate runs and include enough
context (key counts, durations, and error messages) to diagnose failures.
| var expiry = db.KeyExpireTime(key); | ||
| var timeLeft = expiry - now; | ||
| if (!(timeLeft <= TimeSpan.FromMinutes(11))) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Keys without a TTL are silently skipped.
If KeyExpireTime returns null (key has no expiration), the nullable TimeSpan? arithmetic causes the <= check on line 77 to evaluate false, so the key is skipped via continue. This is likely fine if all BRREG cache keys always have a TTL, but worth being aware of — a key that lost its TTL would never be refreshed by this timer.
Consider adding a log statement when expiry is null to aid debugging.
🤖 Prompt for AI Agents
In `@src/Dan.Plugin.Tilda/Functions/Timers.cs` around lines 75 - 80, The code in
Timers.cs using db.KeyExpireTime(key) can return null and currently results in
silently skipping keys (expiry, timeLeft, now); update the logic in the timer
loop (where expiry, timeLeft are computed) to explicitly check for a null expiry
before doing the TimeSpan comparison, and when expiry is null emit a debug/warn
log (using the same logger used in this class) indicating the key has no TTL so
it can be inspected; optionally treat null as requiring refresh (i.e., do not
continue) if that matches intended behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Dan.Plugin.Tilda/Functions/Timers.cs (1)
100-103: Consider logging before early return for observability.When no keys need refreshing, the function exits silently. A brief log line helps confirm the timer ran successfully with nothing to do.
if (mainunitKeys.Count == 0 && subunitKeys.Count == 0) { + logger.LogInformation("CacheRefreshTimer: no keys expiring within threshold, nothing to refresh"); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Dan.Plugin.Tilda/Functions/Timers.cs` around lines 100 - 103, The early-return in Timers.cs (the if checking mainunitKeys.Count == 0 && subunitKeys.Count == 0) exits silently; add a concise log before the return that records the timer ran but found nothing to refresh and include the counts of mainunitKeys and subunitKeys for observability. Use the existing logger instance (e.g., _logger or logger) or inject an ILogger<Timers> if none exists, and log at Information or Debug level inside the same if block immediately before the return so operators can see the timer executed with zero work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Dan.Plugin.Tilda/Functions/Timers.cs`:
- Line 24: The class-level mutable field "logger" is not thread-safe and can be
overwritten when Timer functions MessageToOtherAuditorsTimer or
CacheRefreshTimer run concurrently; change to constructor-injected
ILogger<Timers> (add an ILogger<Timers> parameter to the Timers class
constructor and assign to a readonly field) and update all uses to reference
that injected logger, then remove any context.GetLogger(...) assignments found
in MessageToOtherAuditorsTimer and CacheRefreshTimer and ensure
RefreshOrganisationEntry reads the injected logger instead.
---
Duplicate comments:
In `@src/Dan.Plugin.Tilda/Functions/Timers.cs`:
- Around line 107-118: The current logic creates unbounded concurrent refreshes
by adding RefreshOrganisationEntry tasks for every key in mainunitKeys and
subunitKeys then awaiting Task.WhenAll; replace this with a bounded-concurrency
implementation (e.g., Parallel.ForEachAsync or a SemaphoreSlim-backed loop) that
iterates the combined key set and calls RefreshOrganisationEntry(key,
includeSubunits: ...) with a configured MaxDegreeOfParallelism to throttle HTTP
calls to BRREG; ensure errors from RefreshOrganisationEntry are
observed/aggregated and the includeSubunits flag is set properly per key (use
the existing mainunitKeys and subunitKeys collections and the
RefreshOrganisationEntry method name to locate where to change).
- Around line 81-86: In Timers.cs (the timer refresh loop) the call to
KeyExpireTimeAsync can return null so computing timeLeft and evaluating
!(timeLeft <= TimeSpan.FromMinutes(11)) will include keys with no TTL; update
the logic around expiry/timeLeft (the variables expiry and timeLeft used after
KeyExpireTimeAsync) to explicitly handle null expiry—either skip keys with
expiry == null (or log them) before computing timeLeft, or add a null-coalescing
guard so only keys with a non-null expiry and timeLeft <= 11 minutes are
refreshed; ensure you update the conditional that currently uses timeLeft to
reference the null-checked expiry/timeLeft instead.
---
Nitpick comments:
In `@src/Dan.Plugin.Tilda/Functions/Timers.cs`:
- Around line 100-103: The early-return in Timers.cs (the if checking
mainunitKeys.Count == 0 && subunitKeys.Count == 0) exits silently; add a concise
log before the return that records the timer ran but found nothing to refresh
and include the counts of mainunitKeys and subunitKeys for observability. Use
the existing logger instance (e.g., _logger or logger) or inject an
ILogger<Timers> if none exists, and log at Information or Debug level inside the
same if block immediately before the return so operators can see the timer
executed with zero work.
| IConnectionMultiplexer connectionMultiplexer, | ||
| IBrregService brregService) | ||
| { | ||
| private ILogger logger; |
There was a problem hiding this comment.
Shared mutable logger field is not thread-safe across concurrent timer invocations.
Both MessageToOtherAuditorsTimer (line 33) and CacheRefreshTimer (line 69) write to the instance field logger. If both timers fire concurrently (or either re-enters), one overwrites the other's logger, causing misattributed log entries. Since RefreshOrganisationEntry also reads logger asynchronously, the window is non-trivial.
Inject ILogger<Timers> via the constructor instead, which is the idiomatic pattern for Azure Functions isolated worker with DI:
Proposed fix
public class Timers(
ITildaSourceProvider tildaSourceProvider,
IMtamCounterClient mtamCounterClient,
IAlertMessageSender alertMessageSender,
IConnectionMultiplexer connectionMultiplexer,
- IBrregService brregService)
+ IBrregService brregService,
+ ILogger<Timers> logger)
{
- private ILogger logger;Then remove the logger = context.GetLogger(...) assignments on lines 33 and 69.
Also applies to: 33-33, 69-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Dan.Plugin.Tilda/Functions/Timers.cs` at line 24, The class-level mutable
field "logger" is not thread-safe and can be overwritten when Timer functions
MessageToOtherAuditorsTimer or CacheRefreshTimer run concurrently; change to
constructor-injected ILogger<Timers> (add an ILogger<Timers> parameter to the
Timers class constructor and assign to a readonly field) and update all uses to
reference that injected logger, then remove any context.GetLogger(...)
assignments found in MessageToOtherAuditorsTimer and CacheRefreshTimer and
ensure RefreshOrganisationEntry reads the injected logger instead.
1 participant
Description
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Chores