feat(transport): observability for the import-session cap (register row 53)
Nothing counted open import sessions and nothing logged a rejection at MaxConcurrentImportSessions (default 8), so the register-row-43 session leak could only be inferred from rotating test failures. - Gauge scadabridge.central.transport.import_sessions.open on the existing ZB.MOM.WW.ScadaBridge meter — already in SiteServiceRegistration.ObservedMeters (the ZbTelemetryOptions.Meters allowlist), so no telemetry config change is needed and the series is not silently dropped. Provider-backed, mirroring the StoreAndForward queue-depth gauge: registered by BundleSessionEvictionService .StartAsync (Transport's only lifecycle seam) and identity-checked-cleared on StopAsync so a late stop cannot clobber a newer registrant. - IBundleSessionStore.OpenSessionCount counts only UNEXPIRED sessions. The raw dictionary count would report the node as pinned at the cap for up to a one-minute sweep interval after a burst had drained — the exact false positive the instrument exists to rule out. - BundleSessionStore.Open logs Warning with open count, cap and TTL when it rejects. Fires only on a real cap hit: a store full of expired sessions is reclaimed first and logs nothing. - Docs: docs/components/Transport.md + docs/requirements/Component-Transport.md. Tests: Transport 173 passed (+6 new — Warning content, two negative controls, unexpired-only counting, gauge register/clear, no-clobber-on-late-stop).
This commit is contained in:
@@ -18,6 +18,15 @@ public interface IBundleSessionStore
|
||||
/// <summary>Removes all sessions whose expiry has passed.</summary>
|
||||
void EvictExpired();
|
||||
|
||||
/// <summary>
|
||||
/// Register row 53: the number of sessions currently held that have not yet expired.
|
||||
/// Backs the <c>scadabridge.central.transport.import_sessions.open</c> gauge, so it
|
||||
/// must be cheap and side-effect-free (it is read from the metrics collection thread)
|
||||
/// and must NOT count expired-but-unswept entries — otherwise the gauge would read
|
||||
/// as "at the cap" for up to a sweep interval after a burst drained.
|
||||
/// </summary>
|
||||
int OpenSessionCount { get; }
|
||||
|
||||
/// <summary>
|
||||
/// T-003: returns the current unlock-failure count for a bundle keyed by its
|
||||
/// content hash. The counter is server-owned so a second tab / CLI caller
|
||||
|
||||
@@ -118,6 +118,14 @@ public static class ScadaBridgeTelemetry
|
||||
/// <summary>Provider that yields the live StoreAndForward queue depth; set by a later task.</summary>
|
||||
private static Func<long>? _queueDepthProvider;
|
||||
|
||||
/// <summary>
|
||||
/// Register row 53: provider that yields the number of currently open (unexpired)
|
||||
/// Transport bundle-import sessions, registered by the Transport component's
|
||||
/// session-eviction hosted service. Null on a site node (Transport is central-only),
|
||||
/// where the gauge simply reports 0.
|
||||
/// </summary>
|
||||
private static Func<long>? _importSessionCountProvider;
|
||||
|
||||
#pragma warning disable IDE0052 // Held to keep the observable gauges alive for the meter's lifetime.
|
||||
/// <summary>Gauge reporting the number of currently open site connections.</summary>
|
||||
private static readonly ObservableGauge<long> _siteConnectionUp =
|
||||
@@ -138,6 +146,20 @@ public static class ScadaBridgeTelemetry
|
||||
() => Interlocked.Read(ref _liveAlarmAggregatorsActive),
|
||||
unit: "1",
|
||||
description: "Number of per-site live-alarm aggregators currently running on the active central node.");
|
||||
|
||||
/// <summary>
|
||||
/// Register row 53: gauge reporting the number of open Transport bundle-import
|
||||
/// sessions on this central node. Each open session pins a fully decrypted bundle
|
||||
/// in memory and consumes one of <c>TransportOptions.MaxConcurrentImportSessions</c>
|
||||
/// (default 8), so a series that plateaus at the cap is the signature of leaked
|
||||
/// sessions — the failure mode register row 43 could only be inferred from rotating
|
||||
/// test failures because nothing counted them.
|
||||
/// </summary>
|
||||
private static readonly ObservableGauge<long> _transportImportSessionsOpen =
|
||||
Meter.CreateObservableGauge<long>("scadabridge.central.transport.import_sessions.open",
|
||||
() => Volatile.Read(ref _importSessionCountProvider) is { } p ? p() : 0L,
|
||||
unit: "1",
|
||||
description: "Number of currently open bundle-import sessions on this central node.");
|
||||
#pragma warning restore IDE0052
|
||||
|
||||
// ---------------- Emit helpers ----------------
|
||||
@@ -242,4 +264,42 @@ public static class ScadaBridgeTelemetry
|
||||
// delegate, so a stale stop cannot clobber a successor's provider.
|
||||
Interlocked.CompareExchange(ref _queueDepthProvider, null, provider);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Register row 53: registers the provider the open-import-sessions gauge reads on
|
||||
/// each observation. Called by the Transport component's session-eviction hosted
|
||||
/// service on start. A null provider is ignored so the gauge falls back to 0.
|
||||
/// The callback is invoked synchronously on the metrics collection thread, so it
|
||||
/// must stay cheap — the Transport implementation reads a
|
||||
/// <see cref="System.Collections.Concurrent.ConcurrentDictionary{TKey,TValue}"/> it
|
||||
/// already owns.
|
||||
/// </summary>
|
||||
/// <param name="provider">A callback returning the current open-session count.</param>
|
||||
public static void SetImportSessionCountProvider(Func<long> provider)
|
||||
{
|
||||
if (provider is null)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
Volatile.Write(ref _importSessionCountProvider, provider);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Clears the open-import-sessions provider, but only if the currently registered
|
||||
/// provider is the exact <paramref name="provider"/> delegate passed in
|
||||
/// (reference-equal compare-and-clear), so a late stop of a superseded instance
|
||||
/// cannot stomp a newer registrant. Mirrors <see cref="ClearQueueDepthProvider"/>.
|
||||
/// </summary>
|
||||
/// <param name="provider">The provider delegate to remove; ignored unless it is the
|
||||
/// one currently registered.</param>
|
||||
public static void ClearImportSessionCountProvider(Func<long> provider)
|
||||
{
|
||||
if (provider is null)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
Interlocked.CompareExchange(ref _importSessionCountProvider, null, provider);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
using Microsoft.Extensions.Hosting;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport;
|
||||
using ZB.MOM.WW.ScadaBridge.Commons.Observability;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.Transport.Import;
|
||||
|
||||
@@ -21,6 +22,15 @@ internal sealed class BundleSessionEvictionService : BackgroundService
|
||||
private readonly IBundleSessionStore _sessionStore;
|
||||
private readonly ILogger<BundleSessionEvictionService> _logger;
|
||||
|
||||
/// <summary>
|
||||
/// Register row 53: the open-session gauge callback, held in a field so
|
||||
/// <see cref="StopAsync"/> can pass the SAME delegate reference to
|
||||
/// <see cref="ScadaBridgeTelemetry.ClearImportSessionCountProvider"/> — the clear is
|
||||
/// an identity-checked compare-and-clear, so a stale stop cannot unregister a newer
|
||||
/// instance's provider. Mirrors StoreAndForwardService's queue-depth registration.
|
||||
/// </summary>
|
||||
private readonly Func<long> _openSessionCountProvider;
|
||||
|
||||
/// <summary>
|
||||
/// Initializes a new instance of <see cref="BundleSessionEvictionService"/>.
|
||||
/// </summary>
|
||||
@@ -32,6 +42,35 @@ internal sealed class BundleSessionEvictionService : BackgroundService
|
||||
{
|
||||
_sessionStore = sessionStore ?? throw new ArgumentNullException(nameof(sessionStore));
|
||||
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
|
||||
_openSessionCountProvider = () => _sessionStore.OpenSessionCount;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Register row 53: registers the open-import-sessions gauge provider before the
|
||||
/// sweep loop starts. This service is the Transport component's only lifecycle seam
|
||||
/// (the session store itself is a plain singleton), and it is registered by
|
||||
/// <c>AddTransport</c>, which only the Central branch of the host calls — so the
|
||||
/// gauge reports 0 on a site node, where there are no import sessions to count.
|
||||
/// </summary>
|
||||
/// <param name="cancellationToken">Host startup cancellation token.</param>
|
||||
/// <returns>A task that completes when the base <see cref="BackgroundService"/> has started.</returns>
|
||||
public override Task StartAsync(CancellationToken cancellationToken)
|
||||
{
|
||||
ScadaBridgeTelemetry.SetImportSessionCountProvider(_openSessionCountProvider);
|
||||
return base.StartAsync(cancellationToken);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Deregisters this instance's gauge provider on graceful stop so the gauge falls
|
||||
/// back to 0 rather than reading through a dead store. The clear is identity-checked,
|
||||
/// so a late stop cannot clobber a successor that already re-registered.
|
||||
/// </summary>
|
||||
/// <param name="cancellationToken">Host shutdown cancellation token.</param>
|
||||
/// <returns>A task that completes when the base <see cref="BackgroundService"/> has stopped.</returns>
|
||||
public override async Task StopAsync(CancellationToken cancellationToken)
|
||||
{
|
||||
ScadaBridgeTelemetry.ClearImportSessionCountProvider(_openSessionCountProvider);
|
||||
await base.StopAsync(cancellationToken).ConfigureAwait(false);
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
|
||||
@@ -1,4 +1,6 @@
|
||||
using System.Collections.Concurrent;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
using Microsoft.Extensions.Options;
|
||||
using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport;
|
||||
using ZB.MOM.WW.ScadaBridge.Commons.Types.Transport;
|
||||
@@ -48,6 +50,7 @@ public sealed class BundleSessionStore : IBundleSessionStore
|
||||
|
||||
private readonly TimeProvider _timeProvider;
|
||||
private readonly IOptions<TransportOptions> _options;
|
||||
private readonly ILogger<BundleSessionStore> _logger;
|
||||
|
||||
/// <summary>T-003: per-bundle unlock-failure entry with expiry.</summary>
|
||||
private sealed class UnlockFailureRecord
|
||||
@@ -61,12 +64,42 @@ public sealed class BundleSessionStore : IBundleSessionStore
|
||||
/// </summary>
|
||||
/// <param name="options">Transport options. <see cref="TransportOptions.BundleSessionTtlMinutes"/> is also used as the TTL for the T-003 per-bundle unlock-failure tracker.</param>
|
||||
/// <param name="timeProvider">Time provider used to evaluate session expiry.</param>
|
||||
public BundleSessionStore(IOptions<TransportOptions> options, TimeProvider timeProvider)
|
||||
/// <param name="logger">Optional logger. Register row 53: a rejection at <see cref="TransportOptions.MaxConcurrentImportSessions"/> is logged at Warning, so a session leak is diagnosable from the node log instead of being inferred from rotating test failures.</param>
|
||||
public BundleSessionStore(
|
||||
IOptions<TransportOptions> options,
|
||||
TimeProvider timeProvider,
|
||||
ILogger<BundleSessionStore>? logger = null)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(options);
|
||||
_options = options;
|
||||
_ = options.Value;
|
||||
_timeProvider = timeProvider ?? throw new ArgumentNullException(nameof(timeProvider));
|
||||
_logger = logger ?? NullLogger<BundleSessionStore>.Instance;
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
public int OpenSessionCount
|
||||
{
|
||||
get
|
||||
{
|
||||
// Register row 53: count only unexpired entries. _sessions.Count would
|
||||
// include expired-but-unswept sessions (the eviction sweep runs once a
|
||||
// minute), so the gauge would read "pinned at the cap" for up to a sweep
|
||||
// interval after a burst had actually drained — exactly the false signal
|
||||
// this instrument exists to rule out. Read-only: the gauge callback runs
|
||||
// on the metrics collection thread and must not mutate the store.
|
||||
var now = _timeProvider.GetUtcNow();
|
||||
var count = 0;
|
||||
foreach (var kv in _sessions)
|
||||
{
|
||||
if (kv.Value.ExpiresAt > now)
|
||||
{
|
||||
count++;
|
||||
}
|
||||
}
|
||||
|
||||
return count;
|
||||
}
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
@@ -82,6 +115,17 @@ public sealed class BundleSessionStore : IBundleSessionStore
|
||||
EvictExpired();
|
||||
if (_sessions.Count >= cap)
|
||||
{
|
||||
// Register row 53: this rejection used to be silent apart from the
|
||||
// exception the caller saw. A cap hit is either genuine concurrency
|
||||
// (many operators importing at once) or a session leak like register
|
||||
// row 43's — the count-vs-cap pair plus the recurrence rate in the
|
||||
// log is what distinguishes them.
|
||||
_logger.LogWarning(
|
||||
"Bundle import session rejected: {OpenSessions} of {Cap} concurrent import sessions are open "
|
||||
+ "(ScadaBridge:Transport:MaxConcurrentImportSessions). Sessions release on apply, cancel or "
|
||||
+ "the {TtlMinutes}-minute TTL; a count that stays pinned at the cap indicates leaked sessions.",
|
||||
_sessions.Count, cap, _options.Value.BundleSessionTtlMinutes);
|
||||
|
||||
throw new InvalidOperationException(
|
||||
$"Too many concurrent import sessions ({cap}). Complete or cancel an in-progress import before starting another.");
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user