Skip to content

Commit e7b294a

Browse files
LEGLINK-958: Scope the retry-exhausted purge to acquisition keys only
The retry-exhausted purge deleted the {correlationId} key alongside the {correlationId}:{Type} acquisition keys, on the premise that "nothing downstream will ever consume it." That premise does not hold on this path. ProcessMessageAsync produces ResourcesNormalized BEFORE deleting the acquisition keys, so a failure at or after the produce - a client-side produce timeout the broker actually accepted, or the trailing DeleteAsync throwing - is classed transient and retried while the message is already out. Measure Eval consumes it, finds the patient reportable, and deliberately keeps {correlationId} for its SUPPLEMENTAL pass. When the retries exhaust minutes later, purging that key makes the supplemental read return empty, and Measure Eval "evaluat[es] with empty bundle to produce a not-reportable report" - a silent false answer, not an error. The handler cannot know whether an earlier attempt published; that knowledge died with the attempt, and no transaction spans Kafka and the cache. So the purge is now scoped by what each caller CAN prove: ResourceCachePurgeScope.All Immediate dead-letter path only. DeadLetterException is raised exclusively by ValidateResourcesAcquiredEvent, before the processing loop, so ResourcesNormalized was provably never produced and the correlation key is safe to remove. A comment at the call site pins that invariant: a DeadLetterException thrown after the produce would invalidate it. ResourceCachePurgeScope.AcquisitionKeysOnly Retry exhaustion. Only the keys the message itself carries are deleted; the derived correlation key is left to the cache expiration policy. Cost: one leaked correlation hash per exhausted retry chain. The alternative was a silently wrong clinical report. The scope parameter has no default, so every future call site is forced to answer "can the publish already have happened?" explicitly. Tests: new purger test asserting AcquisitionKeysOnly leaves the correlation key (watched failing with the scope deliberately ignored, proving it detects the old behavior); both call sites' scope choices pinned in their existing tests. Normalization unit tests 40/40. Claude-Session: https://claude.ai/code/session_01AZubeEYCKfAmDCcgVTr18G
1 parent 2752e6d commit e7b294a

6 files changed

Lines changed: 96 additions & 23 deletions

File tree

DotNet/Normalization/Application/Error/ResourcesAcquiredRetryDeadLetterHandler.cs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,10 +80,16 @@ private void PurgeResourceCache(ConsumeResult<string, string> consumeResult, str
8080
return;
8181
}
8282

83+
// Retry exhaustion cannot rule out that an earlier attempt already produced
84+
// ResourcesNormalized and then failed on the trailing cache delete — the produce precedes
85+
// the delete on the success path — in which case Measure Eval is holding {correlationId}
86+
// for its SUPPLEMENTAL pass. Deleting it would make that pass read an empty cache and emit
87+
// a silent false not-reportable report, so only the acquisition keys are released here.
88+
//
8389
// RetryListener runs its consume callback on a background thread with no synchronization
8490
// context, so blocking here cannot deadlock. PurgeAsync does not throw.
8591
_resourceCachePurger
86-
.PurgeAsync(value, $"retry count exhausted: {exceptionMessage}")
92+
.PurgeAsync(value, $"retry count exhausted: {exceptionMessage}", ResourceCachePurgeScope.AcquisitionKeysOnly)
8793
.GetAwaiter()
8894
.GetResult();
8995
}

DotNet/Normalization/Application/Services/ResourceCachePurger.cs

Lines changed: 41 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,30 @@
44

55
namespace LantanaGroup.Link.Normalization.Application.Services;
66

7+
/// <summary>
8+
/// Which cache keys a terminal-failure purge is allowed to remove. The scope encodes what the
9+
/// caller can prove about whether <c>ResourcesNormalized</c> was already published for the message.
10+
/// </summary>
11+
public enum ResourceCachePurgeScope
12+
{
13+
/// <summary>
14+
/// Remove the per-resource-type acquisition keys and the <c>{correlationId}</c> key. Only valid
15+
/// where the failure provably occurred before <c>ResourcesNormalized</c> was produced (the
16+
/// immediate dead-letter path: validation raises before the processing loop), so nothing
17+
/// downstream can be holding the correlation key.
18+
/// </summary>
19+
All,
20+
21+
/// <summary>
22+
/// Remove only the per-resource-type acquisition keys. For paths that cannot rule out a prior
23+
/// publish — retry exhaustion follows attempts that may have produced <c>ResourcesNormalized</c>
24+
/// and then failed on the trailing cache delete — where Measure Eval may already be holding
25+
/// <c>{correlationId}</c> for its SUPPLEMENTAL pass. The correlation key is left to the cache
26+
/// expiration policy.
27+
/// </summary>
28+
AcquisitionKeysOnly
29+
}
30+
731
/// <summary>
832
/// Releases the resource cache entries belonging to a <c>ResourcesAcquired</c> message after a
933
/// terminal (non-retryable) normalization failure.
@@ -13,14 +37,15 @@ namespace LantanaGroup.Link.Normalization.Application.Services;
1337
/// needs its cached resources when it is redelivered, so purging on a transient failure would
1438
/// guarantee the retry fails too.
1539
/// <para>
16-
/// Unlike the success path — which deletes only the per-resource-type acquisition keys and leaves
17-
/// <c>{correlationId}</c> in place for Measure Eval to read — a terminal failure also removes the
18-
/// correlation key, because nothing downstream will ever consume it.
40+
/// The success path deletes only the per-resource-type acquisition keys and leaves
41+
/// <c>{correlationId}</c> in place for Measure Eval to read. Whether a terminal failure may also
42+
/// remove the correlation key depends on what the caller can prove — see
43+
/// <see cref="ResourceCachePurgeScope"/>.
1944
/// </para>
2045
/// </remarks>
2146
public interface IResourceCachePurger
2247
{
23-
Task PurgeAsync(ResourcesAcquiredValue? value, string reason, CancellationToken cancellationToken = default);
48+
Task PurgeAsync(ResourcesAcquiredValue? value, string reason, ResourceCachePurgeScope scope, CancellationToken cancellationToken = default);
2449
}
2550

2651
/// <inheritdoc cref="IResourceCachePurger"/>
@@ -35,7 +60,7 @@ public ResourceCachePurger(IResourceCache resourceCache, ILogger<ResourceCachePu
3560
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
3661
}
3762

38-
public async Task PurgeAsync(ResourcesAcquiredValue? value, string reason, CancellationToken cancellationToken = default)
63+
public async Task PurgeAsync(ResourcesAcquiredValue? value, string reason, ResourceCachePurgeScope scope, CancellationToken cancellationToken = default)
3964
{
4065
var cacheKeys = value?.CacheKeys?.Where(k => !string.IsNullOrWhiteSpace(k)).ToList();
4166

@@ -48,14 +73,18 @@ public async Task PurgeAsync(ResourcesAcquiredValue? value, string reason, Cance
4873
return;
4974
}
5075

51-
// Acquisition keys are "{correlationId}:{ResourceType}"; the correlation key itself holds
52-
// whatever normalization managed to write before it failed, so it goes too.
76+
// Acquisition keys are "{correlationId}:{ResourceType}". The correlation key itself is only
77+
// removed when the caller can prove ResourcesNormalized was never published for this message
78+
// (scope All); otherwise Measure Eval may still be reading it, and it is left to expire.
5379
var keysToDelete = new List<string>(cacheKeys);
54-
keysToDelete.AddRange(cacheKeys
55-
.Select(ExtractCorrelationId)
56-
.Where(correlationId => !string.IsNullOrWhiteSpace(correlationId))
57-
.Distinct()
58-
.Where(correlationId => !keysToDelete.Contains(correlationId)));
80+
if (scope == ResourceCachePurgeScope.All)
81+
{
82+
keysToDelete.AddRange(cacheKeys
83+
.Select(ExtractCorrelationId)
84+
.Where(correlationId => !string.IsNullOrWhiteSpace(correlationId))
85+
.Distinct()
86+
.Where(correlationId => !keysToDelete.Contains(correlationId)));
87+
}
5988

6089
try
6190
{

DotNet/Normalization/Listeners/ResourcesAcquiredListener.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,9 +187,16 @@ public async Task ConsumeMessageAsync(ConsumeResult<ResourceKey, ResourcesAcquir
187187
// Terminal failure: the message is on ResourcesAcquired-Error and will never be normalized,
188188
// so release its cached resources. The retry paths below must NOT do this — a redelivered
189189
// message still needs its cache.
190+
//
191+
// Scope.All is safe here ONLY because DeadLetterException is raised exclusively by
192+
// ValidateResourcesAcquiredEvent, before the processing loop — so ResourcesNormalized was
193+
// provably never produced and nothing downstream can be holding {correlationId}. If a
194+
// DeadLetterException is ever thrown after the produce, this must become
195+
// AcquisitionKeysOnly.
190196
await _resourceCachePurger.PurgeAsync(
191197
result.Message.Value,
192198
$"{nameof(KafkaTopic.ResourcesAcquired)} dead-lettered: {ex.Message}",
199+
ResourceCachePurgeScope.All,
193200
consumeCancellationToken);
194201
}
195202
catch (TransientException ex)

DotNet/ServiceTests/UnitTests/Normalization/ResourceCachePurgerTests.cs

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ public class ResourceCachePurgerTests
1414
private const string CorrelationId = "3fa85f64-5717-4562-b3fc-2c963f66afa6";
1515

1616
[Fact]
17-
public async Task PurgeAsync_DeletesAcquisitionKeysAndCorrelationKey()
17+
public async Task PurgeAsync_ScopeAll_DeletesAcquisitionKeysAndCorrelationKey()
1818
{
1919
var (purger, cache, implementation) = BuildPurger(ResourceCacheType.Redis);
2020

@@ -24,7 +24,7 @@ public async Task PurgeAsync_DeletesAcquisitionKeysAndCorrelationKey()
2424
.Callback<List<string>, CancellationToken>((keys, _) => deletedKeys = keys)
2525
.Returns(Task.CompletedTask);
2626

27-
await purger.PurgeAsync(BuildValue(ResourceCacheType.Redis), "test");
27+
await purger.PurgeAsync(BuildValue(ResourceCacheType.Redis), "test", ResourceCachePurgeScope.All);
2828

2929
cache.Verify(item => item.GetImplementation(ResourceCacheType.Redis), Times.Once);
3030
Assert.NotNull(deletedKeys);
@@ -33,6 +33,30 @@ public async Task PurgeAsync_DeletesAcquisitionKeysAndCorrelationKey()
3333
deletedKeys);
3434
}
3535

36+
[Fact]
37+
public async Task PurgeAsync_ScopeAcquisitionKeysOnly_LeavesTheCorrelationKey()
38+
{
39+
// The retry-exhausted path cannot prove that an earlier attempt did not already publish
40+
// ResourcesNormalized (the produce precedes the acquisition-key delete on the success path),
41+
// so Measure Eval may be holding {correlationId} for its SUPPLEMENTAL pass. That key must
42+
// survive this purge; the cache expiration policy reclaims it if nothing needed it.
43+
var (purger, _, implementation) = BuildPurger(ResourceCacheType.Redis);
44+
45+
List<string>? deletedKeys = null;
46+
implementation
47+
.Setup(item => item.DeleteAsync(It.IsAny<List<string>>(), It.IsAny<CancellationToken>()))
48+
.Callback<List<string>, CancellationToken>((keys, _) => deletedKeys = keys)
49+
.Returns(Task.CompletedTask);
50+
51+
await purger.PurgeAsync(BuildValue(ResourceCacheType.Redis), "test", ResourceCachePurgeScope.AcquisitionKeysOnly);
52+
53+
Assert.NotNull(deletedKeys);
54+
Assert.Equal(
55+
new List<string> { $"{CorrelationId}:Patient", $"{CorrelationId}:Encounter" },
56+
deletedKeys);
57+
Assert.DoesNotContain(CorrelationId, deletedKeys!);
58+
}
59+
3660
[Fact]
3761
public async Task PurgeAsync_UsesTheCacheTypeCarriedOnTheMessage()
3862
{
@@ -42,7 +66,7 @@ public async Task PurgeAsync_UsesTheCacheTypeCarriedOnTheMessage()
4266
.Setup(item => item.DeleteAsync(It.IsAny<List<string>>(), It.IsAny<CancellationToken>()))
4367
.Returns(Task.CompletedTask);
4468

45-
await purger.PurgeAsync(BuildValue(ResourceCacheType.ABS), "test");
69+
await purger.PurgeAsync(BuildValue(ResourceCacheType.ABS), "test", ResourceCachePurgeScope.All);
4670

4771
cache.Verify(item => item.GetImplementation(ResourceCacheType.ABS), Times.Once);
4872
}
@@ -61,7 +85,7 @@ public async Task PurgeAsync_DoesNotDeleteTwiceWhenTheCorrelationKeyIsAlreadyPre
6185
var value = BuildValue(ResourceCacheType.Redis);
6286
value.CacheKeys.Add(CorrelationId);
6387

64-
await purger.PurgeAsync(value, "test");
88+
await purger.PurgeAsync(value, "test", ResourceCachePurgeScope.All);
6589

6690
Assert.NotNull(deletedKeys);
6791
Assert.Equal(deletedKeys!.Count, deletedKeys.Distinct().Count());
@@ -80,7 +104,7 @@ private static async Task AssertNoDelete(List<string>? cacheKeys)
80104
var value = BuildValue(ResourceCacheType.Redis);
81105
value.CacheKeys = cacheKeys!;
82106

83-
await purger.PurgeAsync(value, "test");
107+
await purger.PurgeAsync(value, "test", ResourceCachePurgeScope.All);
84108

85109
cache.Verify(item => item.GetImplementation(It.IsAny<ResourceCacheType>()), Times.Never);
86110
implementation.Verify(
@@ -92,7 +116,7 @@ public async Task PurgeAsync_WithNullValue_DoesNotThrow()
92116
{
93117
var (purger, cache, _) = BuildPurger(ResourceCacheType.Redis);
94118

95-
await purger.PurgeAsync(null, "test");
119+
await purger.PurgeAsync(null, "test", ResourceCachePurgeScope.All);
96120

97121
cache.Verify(item => item.GetImplementation(It.IsAny<ResourceCacheType>()), Times.Never);
98122
}
@@ -107,7 +131,7 @@ public async Task PurgeAsync_WhenDeleteThrows_SwallowsTheException()
107131
.ThrowsAsync(new InvalidOperationException("cache unavailable"));
108132

109133
// The caller is already handling a failed message; cleanup failure must not add another.
110-
await purger.PurgeAsync(BuildValue(ResourceCacheType.Redis), "test");
134+
await purger.PurgeAsync(BuildValue(ResourceCacheType.Redis), "test", ResourceCachePurgeScope.All);
111135
}
112136

113137
private static (ResourceCachePurger, Mock<IResourceCache>, Mock<IResourceCache>) BuildPurger(ResourceCacheType cacheType)

DotNet/ServiceTests/UnitTests/Normalization/ResourcesAcquiredListenerCacheCleanupTests.cs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,10 @@ public async Task ConsumeMessageAsync_DeadLetterFailure_PurgesTheResourceCache()
4444

4545
deadLetterHandler.Verify(
4646
item => item.HandleException(result, It.IsAny<DeadLetterException>(), FacilityId), Times.Once);
47+
// The immediate dead-letter path provably fails before ResourcesNormalized is produced
48+
// (DeadLetterException is raised only by validation), so the full purge is safe here.
4749
purger.Verify(
48-
item => item.PurgeAsync(result.Message.Value, It.IsAny<string>(), It.IsAny<CancellationToken>()),
50+
item => item.PurgeAsync(result.Message.Value, It.IsAny<string>(), ResourceCachePurgeScope.All, It.IsAny<CancellationToken>()),
4951
Times.Once);
5052
}
5153

DotNet/ServiceTests/UnitTests/Normalization/ResourcesAcquiredRetryDeadLetterHandlerTests.cs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,10 @@ public void ProduceDeadLetter_PurgesTheCacheKeysCarriedOnTheRetriedMessage()
4040
};
4141

4242
ResourcesAcquiredValue? purged = null;
43+
ResourceCachePurgeScope? scope = null;
4344
purger
44-
.Setup(item => item.PurgeAsync(It.IsAny<ResourcesAcquiredValue>(), It.IsAny<string>(), It.IsAny<CancellationToken>()))
45-
.Callback<ResourcesAcquiredValue?, string, CancellationToken>((v, _, _) => purged = v)
45+
.Setup(item => item.PurgeAsync(It.IsAny<ResourcesAcquiredValue>(), It.IsAny<string>(), It.IsAny<ResourceCachePurgeScope>(), It.IsAny<CancellationToken>()))
46+
.Callback<ResourcesAcquiredValue?, string, ResourceCachePurgeScope, CancellationToken>((v, _, s, _) => { purged = v; scope = s; })
4647
.Returns(Task.CompletedTask);
4748

4849
handler.ProduceDeadLetter(BuildRetryConsumeResult(JsonSerializer.Serialize(value)), "Retry count exceeded");
@@ -51,6 +52,10 @@ public void ProduceDeadLetter_PurgesTheCacheKeysCarriedOnTheRetriedMessage()
5152
Assert.Equal(value.CacheKeys, purged!.CacheKeys);
5253
Assert.Equal(ResourceCacheType.ABS, purged.CacheType);
5354

55+
// Retry exhaustion cannot prove an earlier attempt did not already publish
56+
// ResourcesNormalized, so it must never remove {correlationId}.
57+
Assert.Equal(ResourceCachePurgeScope.AcquisitionKeysOnly, scope);
58+
5459
// The dead letter is still produced: the durable record of the failure comes first.
5560
producer.Verify(
5661
item => item.Produce(

0 commit comments

Comments
 (0)