guard(di): captive-dependency tripwires — scope validation + an actor field scan

Register row 50. Row 45 (fixed 9e769735) was a live captive dependency —
NotificationOutboxActor cached delivery adapters resolved from an actor-lifetime
IServiceScope, so MaxParallelDeliveries concurrent deliveries shared one EF
DbContext and central-b logged EF's "second operation was started on this context
instance" continuously. Actors take the ROOT IServiceProvider rather than typed
services, which is invisible to build-time analysis, so the bug class had no
mechanical guard at all. This adds both of the register's mitigations; neither
subsumes the other.

1. ValidateScopes + ValidateOnBuild in Development, on BOTH branches, via
   ServiceProviderValidation.Configure. WebApplication.CreateBuilder already sets
   both flags to IsDevelopment(), so on paper this restates a default — it is
   written out because that default is silent (a future move off CreateBuilder
   drops it with no diagnostic) and because this host carries three
   environment-ish signals (SCADABRIDGE_CONFIG, DOTNET_ENVIRONMENT,
   ASPNETCORE_ENVIRONMENT), so "is this node in Development" is not obvious at the
   call site. Production stays unvalidated deliberately: a validation failure is
   fail-closed, and CI now proves what a production node will not check.

   This surfaced two REAL latent violations on the SITE role — not captive
   dependencies but unconstructible descriptors, which fail ValidateOnBuild for
   the entire graph and are therefore what kept the site branch from being
   validatable at all (SiteHealthEndpointTests had pinned UseEnvironment
   ("Production") to work around exactly this):

     - ReconcileService (scoped, needs the central-only IDeploymentManagerRepository)
       moved out of the shared AddCommunication() into a new AddCentralCommunication().
     - AuditLogKpiSampleSource (scoped IKpiSampleSource, needs the central-only
       IAuditLogRepository) moved out of the shared AddAuditLog() into a new
       AddAuditLogCentralKpi().

   Both were registered on sites under the argument "harmless — only the central
   actor ever resolves it". True of the runtime, false of the container: a
   registration nothing can build is what stops the validator being usable, and it
   masks whatever real defect the validator would have found. Both are now
   central-only and SiteHealthEndpointTests boots the site branch validated again.

2. CaptiveDependencyGuard: a reflection scan over every actor class in the shipped
   assemblies (28 today; the reference graph is walked from the Host assembly
   because referenced assemblies load lazily). Fails on any instance field — auto-
   property backing fields included, they are ordinary private fields — typed as
   IServiceScope, as a DbContext, or as a service the REAL composition roots
   register Scoped. The scoped set is derived from the built Central and Site
   containers rather than hand-listed, so a lifetime change automatically re-scopes
   the check. IServiceProvider / IServiceScopeFactory fields are explicitly allowed:
   that IS the correct per-operation-scope pattern.

   No live violation exists — every actor opens a scope per operation. Two fields
   are allowlisted with their safety argument rather than exempted silently:
   AuditLogIngestActor._injectedRepository and SiteCallAuditActor._injectedRepository
   are scoped repository types assigned ONLY by explicitly test-mode constructors,
   null under production wiring (AkkaHostedService uses the IServiceProvider
   overload in both cases), and SiteCallAuditActor additionally wraps its injected
   instance in a SerializedRepository. The shape is one constructor call away from
   being row 45 again, so it is recorded, not dismissed.

Tests: 10 new. Six permanent self-tests of the guard, including positive controls
that reproduce the pre-row-45 shape (an actor caching a scoped service, an
IServiceScope, a DbContext, and a scoped auto-property) and a negative control for
the allowed root-provider pattern — a reflection guard that matches nothing passes
just as green as one that works. Plus an actor-count floor with cross-assembly
spot-checks, an allowlist-staleness test that fails any entry no longer matching a
flagged field, and the two per-role scans. Verified non-vacuous by emptying the
allowlist: the Central scan then names both real fields. Two composition-root
build tests force both validation flags on per role regardless of environment.

Doc: REQ-HOST-11 in Component-Host.md records all three rules and their guards.
This commit is contained in:
Joseph Doherty
2026-08-15 11:58:40 -04:00
parent c6769ee95e
commit 00a804e6d9
9 changed files with 1073 additions and 19 deletions
@@ -248,15 +248,38 @@ public static class ServiceCollectionExtensions
// provider as a singleton on both site and central paths.
sp.GetRequiredService<INodeIdentityProvider>()));
// KPI History & Trends: the AuditLog source the central KPI
// recorder enumerates each sampling pass to snapshot Global-scope Audit
// Log KPIs (volume / error rate / backlog) into the KpiSample history
// table. Scoped to match its IAuditLogRepository dependency — a SCOPED
// EF Core service; the recorder opens a fresh scope per sampling pass.
// TryAdd-Enumerable so multiple sources can register the same interface
// (NotificationOutbox + SiteCallAudit + SiteHealth add their own) without
// any one composition root clobbering another, and re-registration is a
// no-op if AddAuditLog were ever called twice.
return services;
}
/// <summary>
/// Registers the Audit Log's KPI sample source. CENTRAL-ONLY — call from the Central
/// composition root only, alongside <c>AddKpiHistory</c>.
/// </summary>
/// <remarks>
/// <para>
/// KPI History &amp; Trends: the AuditLog source the central KPI recorder enumerates each
/// sampling pass to snapshot Global-scope Audit Log KPIs (volume / error rate / backlog)
/// into the <c>KpiSample</c> history table. Scoped to match its <c>IAuditLogRepository</c>
/// dependency — a SCOPED EF Core service; the recorder opens a fresh scope per sampling
/// pass. TryAdd-Enumerable so multiple sources can register the same interface
/// (NotificationOutbox + SiteCallAudit + SiteHealth add their own) without any one
/// composition root clobbering another, and re-registration is a no-op if this were ever
/// called twice.
/// </para>
/// <para>
/// Split out of <see cref="AddAuditLog"/> by deferred register row 50. Both the KPI
/// recorder and <c>IAuditLogRepository</c> are central-only, so on a site node this
/// registration was unconstructible — enough on its own to fail <c>ValidateOnBuild</c> for
/// the entire site graph and so to keep scope validation switched off there. The site half
/// of <see cref="AddAuditLog"/> (the SQLite writer chain) is unaffected and stays shared.
/// </para>
/// </remarks>
/// <param name="services">The DI service collection to register services into.</param>
/// <returns>The same <see cref="IServiceCollection"/> to allow chaining.</returns>
public static IServiceCollection AddAuditLogCentralKpi(this IServiceCollection services)
{
ArgumentNullException.ThrowIfNull(services);
services.TryAddEnumerable(
ServiceDescriptor.Scoped<IKpiSampleSource, AuditLogKpiSampleSource>());
@@ -42,9 +42,33 @@ public static class ServiceCollectionExtensions
services.AddSingleton<SiteAlarmLiveCacheService>();
services.AddSingleton<ISiteAlarmLiveCache>(sp => sp.GetRequiredService<SiteAlarmLiveCacheService>());
return services;
}
/// <summary>
/// Registers the communication services that only a CENTRAL node can construct.
/// Call alongside <see cref="AddCommunication"/> from the Central composition root only.
/// </summary>
/// <remarks>
/// <para>
/// <see cref="ReconcileService"/> is scoped and takes <c>IDeploymentManagerRepository</c> —
/// an EF repository registered by <c>AddConfigurationDatabase</c>, which only central calls.
/// It used to live in the shared <see cref="AddCommunication"/> on the argument that it was
/// "harmless on site hosts: only the central actor ever resolves it". That is true of the
/// runtime behaviour but not of the container: the registration was UNCONSTRUCTIBLE on a
/// site node, so <c>ValidateOnBuild</c> failed the whole site graph and the site branch
/// could not have scope validation turned on at all (deferred register row 50). A
/// registration nothing can ever build is not harmless — it is the thing that stops the
/// validator from being usable, and it hides whatever real defect the validator would
/// otherwise have found.
/// </para>
/// </remarks>
/// <param name="services">The DI service collection to register services into.</param>
/// <returns>The same <see cref="IServiceCollection"/> to allow chaining.</returns>
public static IServiceCollection AddCentralCommunication(this IServiceCollection services)
{
// Startup reconciliation handler — scoped (holds scoped repositories), resolved
// per-request by CentralCommunicationActor inside a DI scope. Harmless on site
// hosts: only the central actor ever resolves it.
// per-request by CentralCommunicationActor inside a DI scope.
services.AddScoped<ReconcileService>();
return services;
+20
View File
@@ -166,6 +166,10 @@ try
var builder = WebApplication.CreateBuilder(args);
builder.Configuration.AddConfiguration(configuration);
// Captive-dependency validation (deferred register row 50). See the identical
// block in the Site branch below and ServiceProviderValidation for the full rationale.
builder.Host.UseDefaultServiceProvider(ServiceProviderValidation.Configure);
// Serilog
builder.Host.UseSerilog();
@@ -218,6 +222,11 @@ try
// Shared components
builder.Services.AddClusterInfrastructure();
builder.Services.AddCommunication();
// Central-only communication services (ReconcileService — scoped, holds the
// central-only IDeploymentManagerRepository). Split out of AddCommunication by
// deferred register row 50: registering it on a site node produced an
// unconstructible descriptor that failed ValidateOnBuild for the whole site graph.
builder.Services.AddCentralCommunication();
// Per-site gRPC preshared keys. Central-only: it is the side that dials sites, and
// the only side whose key set is dynamic (sites come from the configuration
@@ -301,6 +310,11 @@ try
// the KpiHistoryRecorderActor cluster singleton (started in
// AkkaHostedService). Observability/best-effort: NOT readiness-gated.
builder.Services.AddKpiHistory(builder.Configuration);
// The Audit Log's IKpiSampleSource, enumerated by the recorder registered just
// above. Central-only (its IAuditLogRepository is central-only); split out of
// AddAuditLog by deferred register row 50 for the same reason as
// AddCentralCommunication above.
builder.Services.AddAuditLogCentralKpi();
builder.Services.AddTemplateEngine();
builder.Services.AddDeploymentManager();
// Host is the composition root and owns config-coupled wiring: register the
@@ -683,6 +697,12 @@ try
var builder = WebApplication.CreateBuilder(args);
builder.Configuration.AddConfiguration(configuration);
// Captive-dependency validation (deferred register row 50) — the SITE half of the
// pair. This branch is the one that actually gains coverage: the docker rig sets
// ASPNETCORE_ENVIRONMENT=Development on the central containers but NOT on the site
// containers, so a site node has always built its provider unvalidated.
builder.Host.UseDefaultServiceProvider(ServiceProviderValidation.Configure);
// Serilog
builder.Host.UseSerilog();
@@ -0,0 +1,73 @@
using Microsoft.Extensions.DependencyInjection;
namespace ZB.MOM.WW.ScadaBridge.Host;
/// <summary>
/// The composition root's captive-dependency tripwire: turns .NET's built-in DI
/// scope validation on in Development, for BOTH the Central and Site branches of
/// <c>Program.cs</c>.
/// </summary>
/// <remarks>
/// <para>
/// Deferred register row 50. Row 45 was a live captive dependency — the Notification
/// Outbox actor cached <c>INotificationDeliveryAdapter</c> instances resolved from an
/// actor-lifetime <c>IServiceScope</c>, so up to <c>MaxParallelDeliveries</c> concurrent
/// deliveries shared one EF <c>DbContext</c> and central-b logged EF's
/// "second operation started on this context instance" continuously (fixed in 9e769735).
/// </para>
/// <para>
/// <b>What this catches and what it does not.</b> <see cref="ServiceProviderOptions.ValidateScopes"/>
/// fails a resolve where a SINGLETON directly injects a SCOPED service, and forbids resolving a
/// scoped service from the root provider. <see cref="ServiceProviderOptions.ValidateOnBuild"/>
/// moves the constructor-resolvability half of that check to build time, so a graph defect
/// surfaces as one aggregated boot failure instead of a first-request 500 on a page nobody
/// opened yet. Neither one can see row 45 itself: the actors take the ROOT
/// <see cref="IServiceProvider"/> and call <c>GetRequiredService</c> by hand, which is opaque to
/// static graph validation — that half of row 50 is covered by the reflection guard test
/// (<c>CaptiveDependencyGuard</c> in the Host test suite). The two mitigations are
/// complementary, and neither subsumes the other.
/// </para>
/// <para>
/// <b>Why this is written out rather than left to the framework default.</b>
/// <c>WebApplication.CreateBuilder</c> already sets both flags to
/// <c>IsDevelopment()</c>, so on paper this is a no-op. It is stated explicitly because
/// (a) it is a silent default — nothing in the repo said the property was relied on, and a
/// future move off <c>CreateBuilder</c> (or onto a bare <c>HostBuilder</c>) would drop it with
/// no diagnostic; and (b) this host has THREE environment-ish signals —
/// <c>SCADABRIDGE_CONFIG</c> (role config: Central/Site), <c>DOTNET_ENVIRONMENT</c> (role
/// fallback), and <c>ASPNETCORE_ENVIRONMENT</c> (the actual <see cref="IHostEnvironment"/>) —
/// so "is this node in Development" is genuinely non-obvious at the call site.
/// </para>
/// <para>
/// <b>Production is deliberately excluded.</b> <c>ValidateScopes</c> costs a check on every
/// resolve and <c>ValidateOnBuild</c> costs a full graph walk at boot; more to the point, a
/// validation failure is fail-closed, and a defect that only a rarely-resolved registration
/// would have exposed must not take a production node down at boot when the equivalent signal
/// is available in Development and in CI. The Host test suite forces both flags on for each
/// role regardless of environment, so CI validates what a Production node will not.
/// </para>
/// </remarks>
internal static class ServiceProviderValidation
{
/// <summary>
/// Enables scope + build-time validation when the host environment is Development.
/// Wired via <c>builder.Host.UseDefaultServiceProvider(ServiceProviderValidation.Configure)</c>.
/// </summary>
/// <param name="context">The host builder context, read for its hosting environment.</param>
/// <param name="options">The service-provider options to configure.</param>
internal static void Configure(HostBuilderContext context, ServiceProviderOptions options)
{
Apply(options, context.HostingEnvironment.IsDevelopment());
}
/// <summary>
/// The pure decision, split out so a test can assert both the on and off cases without a host.
/// </summary>
/// <param name="options">The service-provider options to configure.</param>
/// <param name="isDevelopment">Whether the host environment is Development.</param>
internal static void Apply(ServiceProviderOptions options, bool isDevelopment)
{
options.ValidateScopes = isDevelopment;
options.ValidateOnBuild = isDevelopment;
}
}