From d8c6357be83fd415ba7a03c6b7c44c1ed4b298f6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:23:25 -0400 Subject: [PATCH] =?UTF-8?q?fix(r2-02):=20factory=20stamps=20a=20per-Create?= =?UTF-8?q?=20options=20generation=20=E2=80=94=20closes=20the=20respawn=20?= =?UTF-8?q?stale-pipeline=20race=20(01/S-7)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...lience-config-hardening-plan.md.tasks.json | 2 +- .../DriverCapabilityInvokerFactory.cs | 27 ++++++++---- .../DriverCapabilityInvokerFactoryTests.cs | 42 +++++++++++++++++++ 3 files changed, 61 insertions(+), 10 deletions(-) diff --git a/archreview/plans/R2-02-resilience-config-hardening-plan.md.tasks.json b/archreview/plans/R2-02-resilience-config-hardening-plan.md.tasks.json index fa41c229..e79a243d 100644 --- a/archreview/plans/R2-02-resilience-config-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-02-resilience-config-hardening-plan.md.tasks.json @@ -18,7 +18,7 @@ { "id": 14, "subject": "C-7 implement: JsonObject-bag FromJson/ToJson (TagConfigJson idiom), ParseFailed + RawStoredJson, blank->null contract preserved, parser-interop test green", "status": "completed", "blockedBy": [13] }, { "id": 15, "subject": "C-7 razor: ParseFailed warning banner + input lockout + explicit discard button; raw pane shows the stored ResilienceConfig; live-/run on docker-dev :9200 (rebuild BOTH centrals; SQL-mangle + unknown-key survival checks; also drive task 5 warnings and task 9 disabled cells)", "status": "deferred-live", "note": "docker-dev :9200 live-/run; serial live pass", "blockedBy": [14] }, { "id": 16, "subject": "S-7 builder: add options-generation to PipelineKey + GetOrCreate(optionsGeneration=0) + thread through CapabilityInvoker; StaleGeneration_ReCache_IsNotServed_ToNewGeneration", "status": "completed", "blockedBy": [3] }, - { "id": 17, "subject": "S-7 factory: Interlocked per-Create generation stamp + Respawn_interleaved_with_old_invoker_call_still_applies_new_options wiring proof; update Invalidate comment (cleanup, not correctness)", "status": "pending", "blockedBy": [16] }, + { "id": 17, "subject": "S-7 factory: Interlocked per-Create generation stamp + Respawn_interleaved_with_old_invoker_call_still_applies_new_options wiring proof; update Invalidate comment (cleanup, not correctness)", "status": "completed", "blockedBy": [16] }, { "id": 18, "subject": "Wrap-up: whole-solution build (0 warnings, analyzer silent) + full dotnet test gate; record the pass + the two report anchor drifts in archreview/plans/STATUS.md", "status": "pending", "blockedBy": [4, 8, 9, 12, 15, 17] } ], "lastUpdated": "2026-07-12" diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/DriverCapabilityInvokerFactory.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/DriverCapabilityInvokerFactory.cs index dc9919f7..ba014e52 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/DriverCapabilityInvokerFactory.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/DriverCapabilityInvokerFactory.cs @@ -19,11 +19,13 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Resilience; /// /// Options are snapshotted once per (a driver actor's options are fixed /// for its lifetime — a ResilienceConfig change respawns the child), so the invoker's per-call -/// options accessor is allocation-free on the hot path. Because the pipeline builder caches -/// pipelines keyed on (instance, host, capability) and ignores options on a cache hit, -/// first s any pipelines -/// cached for the instance so a respawn with changed options rebuilds them (rather than silently -/// reusing the stale pipeline). On a first spawn this removes nothing. +/// options accessor is allocation-free on the hot path. The pipeline builder caches pipelines keyed on +/// (instance, host, capability, generation) and ignores options on a cache hit; each +/// stamps a fresh monotonic generation (01/S-7) so the new invoker can only +/// read pipelines it built — a lingering old child's post-invalidate re-cache lands under its own older +/// generation and is unreachable. still +/// s the instance's pipelines as cleanup (bounds +/// the cache), but correctness now rests on the generation key, not the invalidate timing. /// public sealed class DriverCapabilityInvokerFactory : IDriverCapabilityInvokerFactory { @@ -31,6 +33,7 @@ public sealed class DriverCapabilityInvokerFactory : IDriverCapabilityInvokerFac private readonly DriverResilienceStatusTracker _statusTracker; private readonly Func _tierResolver; private readonly ILogger? _logger; + private long _generation; /// Construct the factory over the shared resilience infrastructure. /// Process-singleton Polly pipeline builder (shared pipeline cache). @@ -65,9 +68,14 @@ public sealed class DriverCapabilityInvokerFactory : IDriverCapabilityInvokerFac driverInstanceId, driverType, diagnostic); } - // Drop any pipelines cached for this instance under the PREVIOUS options — the builder keys on - // (instance, host, capability) and reuses a cached pipeline regardless of options, so a respawn - // with a changed ResilienceConfig would otherwise keep serving the stale pipeline. No-op on first spawn. + // Stamp a fresh options generation for this invoker. The generation is part of the pipeline-cache + // key (01/S-7), so this invoker can only ever read pipelines it built — a lingering old child's + // late re-cache lands under its OWN (older) generation and is unreachable here. Correctness rests on + // the epoch key, not on the timing of the invalidate below. + var generation = Interlocked.Increment(ref _generation); + + // Cleanup (no longer the correctness mechanism): drop this instance's cached pipelines across all + // generations so lingering old-generation entries don't accumulate. No-op on first spawn. _builder.Invalidate(driverInstanceId); return new CapabilityInvoker( @@ -75,6 +83,7 @@ public sealed class DriverCapabilityInvokerFactory : IDriverCapabilityInvokerFac driverInstanceId, optionsAccessor: () => options, driverType: driverType, - statusTracker: _statusTracker); + statusTracker: _statusTracker, + optionsGeneration: generation); } } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/DriverCapabilityInvokerFactoryTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/DriverCapabilityInvokerFactoryTests.cs index 04aeecc0..329096b3 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/DriverCapabilityInvokerFactoryTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/DriverCapabilityInvokerFactoryTests.cs @@ -161,6 +161,48 @@ public sealed class DriverCapabilityInvokerFactoryTests (e.Message.Contains("timeoutSeconds") || e.Message.Contains("breakerFailureThreshold"))); } + /// + /// 01/S-7 production-wiring proof: a respawn interleaved with a lingering old-invoker call must still + /// apply the new options. The old invoker (a captured reference — exactly what the old child holds) + /// re-caches its stale pipeline AFTER the new 's + /// invalidate; the new invoker must nonetheless behave per the new config. Fails without the per-Create + /// generation stamp (both invokers share the (instance, host, capability) key and the stale entry wins). + /// + [Fact] + public async Task Respawn_interleaved_with_old_invoker_call_still_applies_new_options() + { + var builder = new DriverResiliencePipelineBuilder(); + var factory = MakeFactory(builder); + + // Old invoker: Read retries (tier-A default 3). Warm its cache. + var oldInvoker = factory.Create("d1", "Modbus", "{\"capabilityPolicies\":{\"Read\":{\"retryCount\":3}}}"); + await FailingRead(oldInvoker); // caches the retrying pipeline under the old generation + + // Respawn with new options: Read no longer retries. + var newInvoker = factory.Create("d1", "Modbus", "{\"capabilityPolicies\":{\"Read\":{\"retryCount\":0}}}"); + + // The lingering old child's late call lands AFTER the new Create's invalidate — re-caching the stale + // retrying pipeline under the OLD generation. + await FailingRead(oldInvoker); + + // The new invoker must honor the new options (no retry) — a single attempt. + var attempts = 0; + await Should.ThrowAsync(async () => + await newInvoker.ExecuteAsync( + DriverCapability.Read, "host-1", + async _ => { attempts++; await Task.Yield(); throw new InvalidOperationException("boom"); }, + CancellationToken.None)); + + attempts.ShouldBe(1, "the new invoker must apply the new options (retryCount 0), not the stale re-cache"); + } + + private static async Task FailingRead(IDriverCapabilityInvoker invoker) => + await Should.ThrowAsync(async () => + await invoker.ExecuteAsync( + DriverCapability.Read, "host-1", + async _ => { await Task.Yield(); throw new InvalidOperationException("boom"); }, + CancellationToken.None)); + private sealed class CapturingLogger : ILogger { public List<(LogLevel Level, string Message)> Entries { get; } = new();