dohertj2/lmxopcua
1
0
Fork 0
Code Issues 32 Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(drivers): delete GetMemoryFootprint / FlushOptionalCachesAsync (#525)

Browse Source 
Both `IDriver` members had no production call site. Their only consumer was
`MemoryTracking`, deleted with the driver-tier recycle machinery in #522, and
`GetMemoryFootprint`'s doc-comment claimed "polled every 30s by Core" — which
had not been true since v3, even before #522 removed the poller.

Owner decision on #525 was (a) delete rather than (b) build the process-wide
watchdog that would consume them. The driver-side implementations are in git
history and were mostly reusable; what was always missing is the trigger, and a
member nobody calls is how the misleading doc-comment survived this long.

Removed from `IDriver`, all 12 driver implementations, and every test stub —
plus the support code that existed only to feed them, which is the part a
member-only deletion would have left behind as dead weight:

- MTConnect's three per-entry byte-weight constants, its cached
  `ProbeModelDataItemCount`, and `CountDataItems` — a full device-tree walk run
  on every cache fill to populate a number with no remaining reader.
- OpcUaClient's `_discoveredNodeCount` field and the discovery-pass write to it.
- MQTT's `ApproxBytesPerAuthoredTag`.

Two behavioural notes, neither a change in what the software does:

- `AbCipTemplateCache` now has no production reset path. Its only caller was
  AB CIP's flush, which itself had no caller — so nothing changes except how
  visible the fact is. Recorded on the type, along with the correction that its
  comment's claim about a device `ReinitializeAsync` also flushing it was never
  true. A UDT redefined on a live controller keeps its stale shape until the
  driver instance is rebuilt.
- MTConnect's `/probe` cache is still dropped, by `InvalidateProbeModel()` on an
  Agent restart. That is now `internal` rather than `private` so the tests which
  used the flush purely to empty the cache drive the real path instead — they
  now cover a case that actually occurs in production. Several unrelated doc
  comments cited `FlushOptionalCachesAsync` as the canonical CAS example and are
  repointed to it.

Tests: the ones that existed only to assert on the removed members are deleted
(3 OpcUaClient, 3 Galaxy, 2 MQTT, 1 MTConnect, 1 AbCip). Worth noting one was
vacuous — `FlushOptionalCachesAsync_resets_footprint_counter` asserted a
footprint of 0 before the flush and 0 after with no discovery in between, so it
would have passed against a flush that did nothing at all.

Full suite green except 7 failures verified identical on master before and
after (4 AbLegacy integration with the compose profile down, 3
Host.IntegrationTests), checked by name rather than assumed.

Docs corrected where they described this as live: `DriverLifecycle.md` (which
carried the #525 placeholder), `drivers/AbCip.md`, `drivers/AbLegacy.md`, and
`v2/driver-stability.md` — whose "polls this every 30 s" + per-driver
`DriverConfig` memory budget were never built as written and are now banner-ed
as design intent rather than description.

Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo
This commit is contained in:
Joseph Doherty
2026-07-31 09:07:11 -04:00
parent 803f4e6efe
commit b815446940
42 changed files with 130 additions and 523 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs
+33 -110
View File
@@ -62,21 +62,6 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect;
/// </remarks>
public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDiscovery, IHostConnectivityProbe, IRediscoverable
{
/// <summary>
/// Coarse per-entry byte weights behind <see cref="GetMemoryFootprint"/>. The contract asks
/// for an <i>approximate</i> driver-attributable footprint that Core polls every 30 s to
/// decide whether to ask for a flush, so an order-of-magnitude estimate from the two things
/// that actually scale with a plant (declared data items, live observations) is the right
/// precision. Measuring real retained size would need a heap walk per poll.
/// </summary>
private const int ProbeModelBytesPerDataItem = 512;
/// <summary>Estimated retained bytes per live <c>dataItemId → snapshot</c> entry.</summary>
private const int IndexBytesPerEntry = 256;
/// <summary>Estimated retained bytes per authored <see cref="MTConnectTagDefinition"/>.</summary>
private const int TagBytesPerEntry = 256;
/// <summary>
/// The Agent could not be reached, answered unusably, or blew the per-call deadline. Distinct
/// from the index's <c>BadNoCommunication</c>, which means the Agent answered and said the
@@ -323,15 +308,14 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
/// </summary>
/// <remarks>
/// <para>
/// <b>Why one field instead of three.</b> These three values are read <i>outside</i>
/// <see cref="_lifecycle"/> — by <see cref="ReadAsync"/>, by
/// <see cref="GetOrFetchProbeModelAsync"/> (Task 12's browse), and by
/// <see cref="GetMemoryFootprint"/> — while every write happens under it. As three
/// separate fields they could be observed mid-update: a reader could see a dropped
/// <c>ProbeModel</c> beside a stale non-zero data-item count and report a footprint for
/// a cache that no longer exists, or pair one initialize's client with the previous
/// one's model. Packing them makes every observation internally consistent by
/// construction, with no lock on the read path.
/// <b>Why one field instead of several.</b> These values are read <i>outside</i>
/// <see cref="_lifecycle"/> — by <see cref="ReadAsync"/> and by
/// <see cref="GetOrFetchProbeModelAsync"/> (Task 12's browse) — while every write
/// happens under it. As separate fields they could be observed mid-update, pairing one
/// initialize's client with the previous one's model. Packing them makes every
/// observation internally consistent by construction, with no lock on the read path.
/// (A third value, a cached data-item count, lived here until Gitea #525 deleted its
/// only reader — <c>GetMemoryFootprint</c> — along with the rest of that dead surface.)
/// </para>
/// <para>
/// <b>Why not simply take the lock in those readers.</b> Both readers <c>await</c> the
@@ -423,8 +407,8 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
/// <summary>
/// The cached <c>/probe</c> device model, or <c>null</c> when it has not been fetched or was
/// dropped by <see cref="FlushOptionalCachesAsync"/>. Prefer
/// <see cref="GetOrFetchProbeModelAsync"/>, which cannot observe the flushed hole.
/// dropped by <see cref="InvalidateProbeModel"/> on an Agent restart. Prefer
/// <see cref="GetOrFetchProbeModelAsync"/>, which cannot observe the dropped hole.
/// </summary>
internal MTConnectProbeModel? CachedProbeModel => Volatile.Read(ref _session)?.ProbeModel;
@@ -596,58 +580,6 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
/// <inheritdoc/>
public DriverHealth GetHealth() => ReadHealth();
/// <inheritdoc/>
/// <remarks>
/// Sums the three caches that scale with plant size: the <c>/probe</c> device model (by
/// declared data item), the live observation index (by indexed data item), and the authored
/// tag table. See <see cref="ProbeModelBytesPerDataItem"/> for why the weights are coarse
/// constants.
/// </remarks>
public long GetMemoryFootprint() =>
((long)(Volatile.Read(ref _session)?.ProbeModelDataItemCount ?? 0) * ProbeModelBytesPerDataItem)
+ ((long)ObservationIndex.Count * IndexBytesPerEntry)
+ ((long)Volatile.Read(ref _binding).IndexTags.Count * TagBytesPerEntry);
/// <inheritdoc/>
/// <remarks>
/// Drops the <c>/probe</c> device model — the browse cache, which is re-derivable from the
/// Agent at any time — and keeps the observation index, which is required for correctness
/// (it is the read surface, and re-deriving it would mean a full re-prime the caller did not
/// ask for). Flushing cannot wedge the driver: every consumer of the model goes through
/// <see cref="GetOrFetchProbeModelAsync"/>, which re-fetches and re-caches on a miss.
/// <para>
/// The drop is a single <see cref="Interlocked.CompareExchange{T}"/> of a whole new
/// <see cref="AgentSession"/>, so the model and its data-item count can never be seen
/// out of step (which would skew <see cref="GetMemoryFootprint"/>), and a flush that
/// races a concurrent teardown or re-initialize is discarded rather than resurrecting
/// the session it was reading.
/// </para>
/// </remarks>
public Task FlushOptionalCachesAsync(CancellationToken cancellationToken)
{
var session = Volatile.Read(ref _session);
if (session?.ProbeModel is null)
{
return Task.CompletedTask;
}
var flushed = session with { ProbeModel = null, ProbeModelDataItemCount = 0 };
if (!ReferenceEquals(Interlocked.CompareExchange(ref _session, flushed, session), session))
{
// Someone swapped the session while we were building the flushed copy — their state is
// newer than ours, and publishing would undo it. Whatever they installed is either a
// teardown (no cache to flush) or a fresh initialize (a cache the operator's budget
// breach never saw), so dropping this flush is correct, not merely safe.
return Task.CompletedTask;
}
_logger.LogInformation(
"MTConnect driver {DriverInstanceId} flushed its cached /probe device model ({DataItemCount} data items); it is re-fetched on the next browse.",
_driverInstanceId, session.ProbeModelDataItemCount);
return Task.CompletedTask;
}
/// <inheritdoc/>
/// <remarks>
/// <para>
@@ -963,9 +895,9 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
/// </para>
/// <para>
/// <b>The model comes from <see cref="GetOrFetchProbeModelAsync"/>, never from
/// <see cref="CachedProbeModel"/>.</b> <see cref="FlushOptionalCachesAsync"/> may have
/// dropped the cache under memory pressure; reading the field directly would make a
/// flushed driver browse as an Agent with no data items, permanently.
/// <see cref="CachedProbeModel"/>.</b> <see cref="InvalidateProbeModel"/> may have
/// dropped the cache after an Agent restart; reading the field directly would make such
/// a driver browse as an Agent with no data items, permanently.
/// </para>
/// <para>
/// <b>This throws, and that is the opposite of <see cref="ReadAsync"/>'s posture on
@@ -1250,8 +1182,7 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
/// <para>
/// Publishing the answer into <see cref="AgentSession.ProbeModel"/> is the opposite
/// mistake: it would put a second writer on a field the lifecycle owns under a
/// compare-and-swap, and would silently re-warm a cache that
/// <see cref="FlushOptionalCachesAsync"/> — or an Agent restart, see
/// compare-and-swap, and would silently re-warm a cache that an Agent restart — see
/// <see cref="AnnounceAgentRestart"/> — deliberately dropped. The parsed model is
/// discarded; only the fact that a request completed is used.
/// </para>
@@ -1473,9 +1404,9 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
/// <see cref="DiscoverAsync"/>, which reads
/// <see cref="GetOrFetchProbeModelAsync"/>; with the cache still warm that call answers
/// from a model describing a process that no longer exists, and the "rediscovery" would
/// diff the stale tree against itself and change nothing. Dropped through the same
/// compare-and-swap <see cref="FlushOptionalCachesAsync"/> uses, so a lifecycle change
/// landing mid-drop wins rather than being undone.
/// diff the stale tree against itself and change nothing. Dropped through a
/// compare-and-swap, so a lifecycle change landing mid-drop wins rather than being
/// undone.
/// </para>
/// <para>
/// <b>Once per change, not once per chunk.</b> Every chunk after a restart carries the new
@@ -1514,12 +1445,16 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
}
/// <summary>
/// Drops the cached <c>/probe</c> device model so the next consumer re-fetches it. Same
/// compare-and-swap discipline as <see cref="FlushOptionalCachesAsync"/> — retried, because
/// unlike a flush this drop is a correctness requirement rather than a courtesy: losing the
/// race must not leave a model from a dead Agent installed.
/// Drops the cached <c>/probe</c> device model so the next consumer re-fetches it.
/// <c>internal</c> rather than <c>private</c> so the discovery tests can exercise the real
/// drop path; production calls it from <see cref="AnnounceAgentRestart"/>.
/// Compare-and-swap, and <b>retried</b> — this drop is a correctness requirement rather
/// than a courtesy: losing the race must not leave a model from a dead Agent installed.
/// (The courtesy version, <c>FlushOptionalCachesAsync</c>, took the same lock-free
/// approach but gave up on a lost race; it went with the rest of the unconsumed
/// memory-pressure surface in Gitea #525.)
/// </summary>
private void InvalidateProbeModel()
internal void InvalidateProbeModel()
{
while (true)
{
@@ -1529,7 +1464,7 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
return;
}
var dropped = session with { ProbeModel = null, ProbeModelDataItemCount = 0 };
var dropped = session with { ProbeModel = null };
if (ReferenceEquals(Interlocked.CompareExchange(ref _session, dropped, session), session))
{
return;
@@ -2191,7 +2126,7 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
/// sequence that had just been rejected.
/// </para>
/// <para>
/// Compare-and-swap for the same reason <see cref="FlushOptionalCachesAsync"/> uses one:
/// Compare-and-swap for the same reason <see cref="InvalidateProbeModel"/> uses one:
/// a lifecycle change landing mid-update has newer state, so this write is dropped
/// rather than replayed over it.
/// </para>
@@ -2221,7 +2156,7 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
/// The <c>/probe</c> device model: the cached copy when warm, otherwise a fresh
/// deadline-bounded <c>/probe</c> that re-populates the cache. This is the accessor every
/// model consumer (Task 12's <c>DiscoverAsync</c>) must use, so that
/// <see cref="FlushOptionalCachesAsync"/> can never leave browse permanently disabled.
/// <see cref="InvalidateProbeModel"/> can never leave browse permanently disabled.
/// </summary>
/// <remarks>
/// Lock-free by design — see the <see cref="_session"/> remarks for why this does not take
@@ -2261,7 +2196,7 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
}
// Publish only onto the session we actually fetched against.
var recached = session with { ProbeModel = model, ProbeModelDataItemCount = CountDataItems(model) };
var recached = session with { ProbeModel = model };
_ = Interlocked.CompareExchange(ref _session, recached, session);
return model;
@@ -2416,7 +2351,7 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
// cache, and the pump's opening cursor together or none of them.
Volatile.Write(
ref _session,
new AgentSession(client, probe, CountDataItems(probe), current.InstanceId, current.NextSequence));
new AgentSession(client, probe, current.InstanceId, current.NextSequence));
var index = new MTConnectObservationIndex(binding.IndexTags);
index.Apply(current);
@@ -2738,13 +2673,6 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
|| current.SampleIntervalMs != incoming.SampleIntervalMs
|| current.SampleCount != incoming.SampleCount;
/// <summary>
/// Counts every data item the device model declares. Done once, when the model is cached,
/// rather than re-walking the tree on every 30 s <see cref="GetMemoryFootprint"/> poll.
/// </summary>
private static int CountDataItems(MTConnectProbeModel model) =>
model.Devices.Sum(d => d.AllDataItems().Count());
/// <summary>
/// Runs one unary agent call under a deadline linked to the caller's token. Belt-and-braces
/// over the production client's own per-call deadline: the seam does not oblige an
@@ -3196,17 +3124,13 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
/// <summary>
/// The live agent client and the <c>/probe</c> cache derived from it, as one immutable
/// unit. Immutable so that publishing a change is a single reference swap — see the
/// <see cref="_session"/> remarks for why the three values must move together.
/// <see cref="_session"/> remarks for why these values must move together.
/// </summary>
/// <param name="Client">The agent client this session owns and will dispose on teardown.</param>
/// <param name="ProbeModel">
/// The cached device model, or <c>null</c> once <see cref="FlushOptionalCachesAsync"/> has
/// The cached device model, or <c>null</c> once <see cref="InvalidateProbeModel"/> has
/// dropped it. Never null at the moment a session is first published.
/// </param>
/// <param name="ProbeModelDataItemCount">
/// Data items declared by <paramref name="ProbeModel"/>, counted once at cache time and
/// always <c>0</c> when it is <c>null</c>.
/// </param>
/// <param name="InstanceId">
/// The Agent's <c>instanceId</c> as last observed. Updated in place (by
/// <see cref="PublishAgentCursor"/>) when the pump sees the Agent restart, so the
@@ -3220,7 +3144,6 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis
private sealed record AgentSession(
IMTConnectAgentClient Client,
MTConnectProbeModel? ProbeModel,
int ProbeModelDataItemCount,
long InstanceId,
long NextSequence);