LEGLINK-948: Recover Redis used memory before falling back to ABS

[MikeAtPinnacle]

🛠️ Description of Changes

Fixes and instruments the Hybrid resource cache selection for LEGLINK-948, where ResourceCache:CacheImplementation = Hybrid selects ABS on every correlation in TEST.

The reporter's A/B/A reproduction rules out connectivity: forcing CacheImplementation = Redis immediately before and after the failing run writes to Redis successfully. That narrows the problem to HybridResourceCache.SelectCacheTypeAsync, which had two defects.

1. The memory probe gave up on the first miss.

SelectCacheTypeAsync sourced used_memory from INFO memory and nothing else. If that section came back empty it selected ABS; if INFO threw, the outer catch selected ABS. Either way a restricted command decided the cache rather than memory pressure. Azure Managed Redis proxies Redis Enterprise and already withholds maxmemory — that is precisely why LEGLINK-770 moved the limit into configuration — so the same restrictions can plausibly hide used_memory or the whole INFO memory section. On such an instance the old code produces deterministic always-ABS, which is the reported symptom.

The numerator is now recovered through a chain, and is still measured against the configured ResourceCache:Redis:MaxMemoryBytes. Only the numerator ever comes from the server:

INFO memory → used_memory
  └─ missing, empty, or rejected? → MEMORY STATS total.allocated
       └─ missing or rejected? → full INFO dump, scan for used_memory
            └─ still nothing → ABS (logged as a warning)

then: usedMemory / MaxMemoryBytes >= threshold ? ABS : Redis

A rejected INFO no longer reaches the outer catch — ReadInfoAsync swallows non-cancellation exceptions and returns null so the chain continues. The outer catch still routes genuine connectivity failures to ABS.

Selection behaviour, before and after:

Situation	Before	After
used_memory present	percentage vs configured max	unchanged
used_memory absent or unparsable	Redis, immediately	consult MEMORY STATS, then full INFO
INFO memory section empty	ABS, immediately	consult MEMORY STATS, then full INFO
INFO rejected or throws	ABS, via outer catch	consult MEMORY STATS, then full INFO
every source exhausted	n/a	ABS
no connected server	ABS	unchanged
MaxMemoryBytes unset	Redis	unchanged

2. The decision was invisible in every deployed environment.

Four paths returned ABS and three logged at Debug. Deployed environments run at Information, so an ABS result could not be distinguished from a failed probe without redeploying at a different log level — the reason this bug needed a code change before it could even be diagnosed.

• The decision is logged at Information for both outcomes with the used-memory figure, which command produced it, the configured MaxMemoryBytes, the computed percentage and the threshold.
• Every fall-through and failure path logs a Warning naming what failed, with the endpoint and the reported memory statistics.
• When no connected server is found, the configured endpoints and per-server IsConnected state are reported.
• The full INFO memory section is dumped once per process, so the raw server output can be inspected without shell access to the Redis instance. maxmemory is included deliberately: AMR not reporting it is the assumption behind LEGLINK-770, and this confirms whether that still holds per instance.

SelectCacheTypeAsync is memoized per correlationId, so it runs roughly once per patient-correlation rather than per resource. Information here is not a hot path.

3. ToDictionary replaced with a defensive lookup. It throws on duplicate keys and the catch-all swallowed that into a silent ABS fallback. AMR is not guaranteed to return the unique-key INFO shape open-source Redis does.

Once deployed, the decision line distinguishes the remaining hypotheses. via INFO memory with a figure close to what Redis Insight reports means real memory pressure against a mis-sized denominator; via MEMORY STATS or via INFO (full) means INFO memory is restricted on AMR; a figure far above what Redis Insight reports means the metric is node-level and the percentage approach is unsound there; and the exhausted-sources warning means no memory signal is available at all.

Note this lives in DotNet/Shared, so every .NET service picks it up, not only the three services in the reproduction steps.

Related follow-up, not in this PR: ResourceCache:Redis:MaxMemoryBytes is unprovisioned in dev, qa, qa2 and test, so all environments fall back to the appsettings.json default of 268435456 (256 MB) while app-config.yaml documents LCG Redis instances as 1 GB. Deliberately left out so the first run after this deploys reads the current denominator — changing both at once would blur which factor moved the result.

🧪 Testing Performed

• dotnet build DotNet/Shared/Shared.csproj — clean, 0 errors.
• dotnet test DotNet/ServiceTests/ServiceTests.csproj --filter FullyQualifiedName~HybridResourceCacheTests — 15/15 passing (9 pre-existing, 6 new).
• Confirmed the new lines are emitted where they are needed: DataAcquisition, DataAcquisition.AcquisitionWorker and Normalization all run Serilog at MinimumLevel.Default = Information with overrides only for Microsoft and System, and the TEST App Configuration store sets the same three keys. LantanaGroup.Link.Shared.* is not filtered, so these reach Loki.

Not yet exercised against a live Azure Managed Redis instance. That is the next step: deploy to TEST and re-run the reporter's reproduction steps. The INFO memory dump and the via <command> marker on the decision line are what confirm which commands that instance actually answers.

🧑‍🔬 Unit Testing

• I have written or updated unit tests to cover my changes
• Coverage: 91.3%

📓 Documentation Updated

No documentation changes needed. No configuration keys are added, removed or renamed, and the meaning of ResourceCache:Redis:MaxMemoryBytes and MemoryThresholdPercent is unchanged — the percentage comparison they drive is the same, only the recovery of the used-memory numerator and the observability around it have changed.

[coderabbitai]

📝 Walkthrough

Walkthrough

HybridResourceCache now logs Redis probe and cache-selection details, parses duplicate Redis INFO keys safely, and retains defined Redis or ABS fallback behavior. Unit tests verify log levels, disconnected-server warnings, and duplicate used_memory handling.

Changes

Hybrid resource cache diagnostics

Layer / File(s)	Summary
Redis diagnostic parsing DotNet/Shared/Application/Services/ResourceCache/HybridResourceCache.cs	Adds curated memory diagnostics, duplicate-safe case-insensitive parsing, endpoint and server-state descriptions, and one-time raw INFO memory logging.
Probe and selection logging DotNet/Shared/Application/Services/ResourceCache/HybridResourceCache.cs	Logs Redis probe results, memory diagnostics, threshold calculations, selected cache type, and endpoint-specific exceptions. Missing or invalid memory data remains on the Redis path; unavailable servers and probe failures use ABS.
Logging and selection tests DotNet/ServiceTests/UnitTests/Shared/ResourceCache/HybridResourceCacheTests.cs	Adds generalized log verification and tests for information-level selection logs, disconnected-server warnings, and duplicate used_memory entries.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• lantanagroup/link-cloud#1764: Introduced the HybridResourceCache selection logic extended by this change.
• lantanagroup/link-cloud#1770: Also modifies Redis memory probing and selection diagnostics.
• lantanagroup/link-cloud#1771: Introduced related asynchronous Redis selection behavior.

Suggested reviewers: johnbritton

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the Redis memory recovery and ABS fallback change described in the pull request.
Description check	✅ Passed	The description covers the required change overview, testing, unit-test coverage, and documentation status with detailed implementation context.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch users/mtherien/leglink-948-change-logging

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

DotNet/ServiceTests/UnitTests/Shared/ResourceCache/HybridResourceCacheTests.cs (1)

169-215: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add focused tests for the remaining probe branches.

Add one XUnit test where InfoAsync("memory") returns no section and verify ABS is selected. Add one XUnit test where used_memory is non-numeric and verify Redis is selected with a warning. Keep the tests mock-only.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@DotNet/ServiceTests/UnitTests/Shared/ResourceCache/HybridResourceCacheTests.cs`
around lines 169 - 215, Add two mock-only XUnit tests alongside the existing
HybridResourceCache tests: configure InfoAsync("memory") to return no section
and verify writes use _absCache, then configure a non-numeric used_memory value
and verify writes use _redisCache while VerifyWarningLogged confirms a warning.
Reuse CreateSut, Write, and the existing Redis setup helpers without changing
production code.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@DotNet/ServiceTests/UnitTests/Shared/ResourceCache/HybridResourceCacheTests.cs`:
- Around line 175-183: Strengthen Selection_decision_is_logged_at_Information by
asserting the log message content, not only its Information level. Update
VerifyLogged to require the selection marker “=> selected” and the chosen cache
type “Redis”, while preserving the existing at-least-once expectation.

In `@DotNet/Shared/Application/Services/ResourceCache/HybridResourceCache.cs`:
- Line 49: The _infoSectionLogged guard in HybridResourceCache must be
process-wide rather than instance-specific. Change the field declaration to
static so all HybridResourceCache instances share the same logging state,
preserving the existing once-only INFO section logging behavior.
- Around line 276-280: Sanitize every dynamic logging argument in
HybridResourceCache before passing it to logger methods: at lines 276-280
sanitize the full Redis INFO section; at lines 157-161 sanitize the configured
endpoint and server-state descriptions; at lines 171-188 sanitize the endpoint,
raw used_memory, and formatted diagnostics; at lines 197-221 sanitize the
endpoint and diagnostic strings; and at lines 227-230 sanitize the endpoint. Use
the existing project sanitization utility or established pattern consistently
across these calls.

---

Nitpick comments:
In
`@DotNet/ServiceTests/UnitTests/Shared/ResourceCache/HybridResourceCacheTests.cs`:
- Around line 169-215: Add two mock-only XUnit tests alongside the existing
HybridResourceCache tests: configure InfoAsync("memory") to return no section
and verify writes use _absCache, then configure a non-numeric used_memory value
and verify writes use _redisCache while VerifyWarningLogged confirms a warning.
Reuse CreateSut, Write, and the existing Redis setup helpers without changing
production code.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a3d76326-e686-4edd-85e7-572204983c44

📥 Commits

Reviewing files that changed from the base of the PR and between f220e0c and 3947d83.

📒 Files selected for processing (2)

• DotNet/ServiceTests/UnitTests/Shared/ResourceCache/HybridResourceCacheTests.cs
• DotNet/Shared/Application/Services/ResourceCache/HybridResourceCache.cs