00a804e6d9
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.
288 lines
13 KiB
C#
288 lines
13 KiB
C#
using System.Reflection;
|
|
using Akka.Actor;
|
|
using Microsoft.EntityFrameworkCore;
|
|
using Microsoft.Extensions.DependencyInjection;
|
|
|
|
namespace ZB.MOM.WW.ScadaBridge.Host.Tests;
|
|
|
|
/// <summary>
|
|
/// Deferred register row 50, mitigation 2: a reflection guard that no Akka actor holds
|
|
/// captive scope state — a scoped service, or an <see cref="IServiceScope"/>, cached for
|
|
/// the actor's lifetime.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// <para>
|
|
/// <b>Why this exists separately from scope validation.</b> Register row 45 was exactly this
|
|
/// bug: <c>NotificationOutboxActor</c> cached delivery adapters resolved from an
|
|
/// actor-lifetime <c>IServiceScope</c>, so <c>MaxParallelDeliveries</c> concurrent deliveries
|
|
/// shared one EF <c>DbContext</c> and EF threw "a second operation was started on this
|
|
/// context instance" continuously on the rig (fixed 9e769735). .NET's own
|
|
/// <c>ValidateScopes</c> could never have caught it: the actors take the ROOT
|
|
/// <see cref="IServiceProvider"/> and call <c>GetRequiredService</c> by hand, which the
|
|
/// container's static graph analysis cannot see through. This guard closes that half.
|
|
/// </para>
|
|
/// <para>
|
|
/// <b>What is allowed.</b> Holding <see cref="IServiceProvider"/> or
|
|
/// <see cref="IServiceScopeFactory"/> for the actor's lifetime is the CORRECT pattern and is
|
|
/// deliberately not flagged — every actor here does that and opens a fresh scope per
|
|
/// operation. What is forbidden is holding the SCOPE, or anything resolved from one, past
|
|
/// the operation that opened it.
|
|
/// </para>
|
|
/// <para>
|
|
/// <b>How "scoped" is decided.</b> The caller supplies the scoped set derived from the actual
|
|
/// DI registrations of a real composition root, so the guard tracks the container rather than
|
|
/// a hand-maintained list — re-registering a service from singleton to scoped automatically
|
|
/// brings any actor field of that type into scope for the check. Two types are flagged
|
|
/// unconditionally on top of that set, because they are wrong in an actor field whatever the
|
|
/// container says: <see cref="IServiceScope"/> itself, and any <see cref="DbContext"/>.
|
|
/// </para>
|
|
/// <para>
|
|
/// Auto-property backing fields need no special handling: the compiler emits them as private
|
|
/// instance fields, so <c>GetFields(NonPublic | Instance)</c> already returns them.
|
|
/// </para>
|
|
/// </remarks>
|
|
internal static class CaptiveDependencyGuard
|
|
{
|
|
/// <summary>One offending field on one actor type.</summary>
|
|
/// <param name="ActorType">The actor class declaring the field.</param>
|
|
/// <param name="FieldName">The field's name (a compiler-generated backing field keeps its mangled name).</param>
|
|
/// <param name="FieldType">The field's declared type.</param>
|
|
/// <param name="Reason">Why the field was flagged.</param>
|
|
internal sealed record Violation(Type ActorType, string FieldName, Type FieldType, string Reason)
|
|
{
|
|
/// <inheritdoc/>
|
|
public override string ToString() =>
|
|
$"{ActorType.FullName}.{FieldName} : {FieldType.FullName} — {Reason}";
|
|
}
|
|
|
|
/// <summary>
|
|
/// Field types that are always correct to hold for an actor's lifetime: they ARE the
|
|
/// per-operation-scope pattern rather than a violation of it.
|
|
/// </summary>
|
|
internal static readonly IReadOnlyList<Type> AllowedProviderTypes =
|
|
[
|
|
typeof(IServiceProvider),
|
|
typeof(IServiceScopeFactory),
|
|
];
|
|
|
|
/// <summary>
|
|
/// Fields that hold a scoped-typed value but are provably safe, keyed
|
|
/// <c>"Namespace.ActorType.fieldName"</c>. Every entry carries its safety argument —
|
|
/// nothing is exempted silently.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// <para>
|
|
/// Both entries below are the same shape: a nullable repository field assigned ONLY by an
|
|
/// explicitly test-mode constructor, so the production wiring
|
|
/// (<c>AkkaHostedService</c> constructs both actors through their <see cref="IServiceProvider"/>
|
|
/// overload) leaves the field null and every handler takes the create-a-scope branch. The
|
|
/// test fixtures that do use those constructors are MSSQL-backed TestKit fixtures that
|
|
/// drive the actor one message at a time, and <c>SiteCallAuditActor</c> additionally wraps
|
|
/// the injected instance in its own <c>SerializedRepository</c> — i.e. the test path
|
|
/// already carries a serialization guard for precisely the concurrency hazard row 45 hit.
|
|
/// </para>
|
|
/// <para>
|
|
/// They are allowlisted rather than removed because deleting a test-mode constructor is a
|
|
/// larger change than this row justifies, and rather than left unflagged because the shape
|
|
/// is genuinely one constructor call away from being row 45 again: if a production call
|
|
/// site ever switched to the repository overload, the actor would hold a scoped EF-backed
|
|
/// repository for its whole lifetime. That residual risk is recorded, not dismissed.
|
|
/// </para>
|
|
/// </remarks>
|
|
internal static readonly IReadOnlySet<string> Allowlist = new HashSet<string>(StringComparer.Ordinal)
|
|
{
|
|
"ZB.MOM.WW.ScadaBridge.AuditLog.Central.AuditLogIngestActor._injectedRepository",
|
|
"ZB.MOM.WW.ScadaBridge.SiteCallAudit.SiteCallAuditActor._injectedRepository",
|
|
};
|
|
|
|
/// <summary>
|
|
/// Scans the given actor types for instance fields holding captive scope state.
|
|
/// </summary>
|
|
/// <param name="actorTypes">Actor classes to inspect.</param>
|
|
/// <param name="scopedServiceTypes">
|
|
/// Service and implementation types registered with <see cref="ServiceLifetime.Scoped"/>
|
|
/// in the composition root being checked.
|
|
/// </param>
|
|
/// <param name="applyAllowlist">
|
|
/// False makes the scan report allowlisted fields too — used by the guard's own self-tests
|
|
/// so the allowlist cannot quietly grow to cover a real regression.
|
|
/// </param>
|
|
/// <returns>Every violation found, in declaration order.</returns>
|
|
internal static IReadOnlyList<Violation> Scan(
|
|
IEnumerable<Type> actorTypes,
|
|
IReadOnlySet<Type> scopedServiceTypes,
|
|
bool applyAllowlist = true)
|
|
{
|
|
ArgumentNullException.ThrowIfNull(actorTypes);
|
|
ArgumentNullException.ThrowIfNull(scopedServiceTypes);
|
|
|
|
var violations = new List<Violation>();
|
|
|
|
foreach (var actorType in actorTypes)
|
|
{
|
|
foreach (var field in InstanceFields(actorType))
|
|
{
|
|
var fieldType = field.FieldType;
|
|
|
|
if (AllowedProviderTypes.Contains(fieldType))
|
|
{
|
|
// The correct pattern — create a scope per operation.
|
|
continue;
|
|
}
|
|
|
|
string? reason = null;
|
|
if (fieldType == typeof(IServiceScope) || fieldType == typeof(AsyncServiceScope))
|
|
{
|
|
reason = "holds a DI scope for the actor's lifetime; open a scope per operation instead";
|
|
}
|
|
else if (typeof(DbContext).IsAssignableFrom(fieldType))
|
|
{
|
|
reason = "holds an EF DbContext for the actor's lifetime; resolve it inside a per-operation scope";
|
|
}
|
|
else if (scopedServiceTypes.Contains(fieldType))
|
|
{
|
|
reason = "holds a service registered Scoped; resolve it inside a per-operation scope";
|
|
}
|
|
|
|
if (reason is null)
|
|
{
|
|
continue;
|
|
}
|
|
|
|
if (applyAllowlist && Allowlist.Contains($"{actorType.FullName}.{field.Name}"))
|
|
{
|
|
continue;
|
|
}
|
|
|
|
violations.Add(new Violation(actorType, field.Name, fieldType, reason));
|
|
}
|
|
}
|
|
|
|
return violations;
|
|
}
|
|
|
|
/// <summary>
|
|
/// Every instance field declared by the actor class and by its ScadaBridge-owned base
|
|
/// classes. The walk stops at the first non-ScadaBridge base, which is how Akka's own
|
|
/// <see cref="ActorBase"/> internals are excluded without naming them.
|
|
/// </summary>
|
|
/// <param name="actorType">The actor class to walk.</param>
|
|
/// <returns>The instance fields, most-derived first.</returns>
|
|
private static IEnumerable<FieldInfo> InstanceFields(Type actorType)
|
|
{
|
|
for (var t = actorType; t is not null && IsScadaBridgeType(t); t = t.BaseType)
|
|
{
|
|
foreach (var field in t.GetFields(
|
|
BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly))
|
|
{
|
|
yield return field;
|
|
}
|
|
}
|
|
}
|
|
|
|
/// <summary>Whether a type belongs to a first-party ScadaBridge assembly.</summary>
|
|
/// <param name="type">The type to test.</param>
|
|
/// <returns>True when the declaring assembly is a ScadaBridge assembly.</returns>
|
|
private static bool IsScadaBridgeType(Type type) =>
|
|
type.Assembly.GetName().Name?.StartsWith(AssemblyPrefix, StringComparison.Ordinal) == true;
|
|
|
|
private const string AssemblyPrefix = "ZB.MOM.WW.ScadaBridge";
|
|
|
|
/// <summary>
|
|
/// Every concrete Akka actor class in the shipped ScadaBridge assemblies. Test assemblies
|
|
/// are excluded deliberately — the guard's own deliberately-violating fixtures live there,
|
|
/// and a test-only actor is not part of any production composition root.
|
|
/// </summary>
|
|
/// <returns>The production actor types, ordered by full name.</returns>
|
|
internal static IReadOnlyList<Type> ProductionActorTypes() =>
|
|
ProductionAssemblies()
|
|
.SelectMany(a => a.GetTypes())
|
|
.Where(t => typeof(ActorBase).IsAssignableFrom(t) && t is { IsAbstract: false, IsInterface: false })
|
|
.OrderBy(t => t.FullName, StringComparer.Ordinal)
|
|
.ToList();
|
|
|
|
/// <summary>
|
|
/// Transitively loads and returns the shipped ScadaBridge assemblies.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// Referenced assemblies load lazily, so enumerating <c>AppDomain.CurrentDomain</c> alone
|
|
/// would silently scan whichever subset the test run happened to touch — a guard that
|
|
/// quietly checks nothing. The reference graph is walked from the Host assembly instead,
|
|
/// and the caller asserts a floor on the actor count so a regression here fails loudly.
|
|
/// </remarks>
|
|
/// <returns>The distinct production ScadaBridge assemblies.</returns>
|
|
private static IReadOnlyList<Assembly> ProductionAssemblies()
|
|
{
|
|
var seen = new Dictionary<string, Assembly>(StringComparer.Ordinal);
|
|
var pending = new Stack<Assembly>();
|
|
pending.Push(typeof(Program).Assembly);
|
|
|
|
while (pending.Count > 0)
|
|
{
|
|
var assembly = pending.Pop();
|
|
var name = assembly.GetName().Name;
|
|
if (name is null
|
|
|| !name.StartsWith(AssemblyPrefix, StringComparison.Ordinal)
|
|
|| name.EndsWith(".Tests", StringComparison.Ordinal)
|
|
|| !seen.TryAdd(name, assembly))
|
|
{
|
|
continue;
|
|
}
|
|
|
|
foreach (var reference in assembly.GetReferencedAssemblies())
|
|
{
|
|
if (reference.Name?.StartsWith(AssemblyPrefix, StringComparison.Ordinal) != true
|
|
|| seen.ContainsKey(reference.Name))
|
|
{
|
|
continue;
|
|
}
|
|
|
|
try
|
|
{
|
|
pending.Push(Assembly.Load(reference));
|
|
}
|
|
catch (Exception ex) when (ex is FileNotFoundException or BadImageFormatException)
|
|
{
|
|
// A ScadaBridge assembly that is referenced but not deployed alongside the
|
|
// test binaries cannot contribute actors to this run either.
|
|
}
|
|
}
|
|
}
|
|
|
|
return seen.Values.ToList();
|
|
}
|
|
|
|
/// <summary>
|
|
/// Collects the service and implementation types registered with
|
|
/// <see cref="ServiceLifetime.Scoped"/> in a composition root.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// Both sides of a descriptor are collected: an actor can just as easily cache the
|
|
/// concrete <c>EmailNotificationDeliveryAdapter</c> as the <c>INotificationDeliveryAdapter</c>
|
|
/// it is registered as, and row 45 in fact cached the interface.
|
|
/// </remarks>
|
|
/// <param name="services">The composition root's service collection.</param>
|
|
/// <returns>The scoped service and implementation types.</returns>
|
|
internal static IReadOnlySet<Type> ScopedTypes(IEnumerable<ServiceDescriptor> services)
|
|
{
|
|
ArgumentNullException.ThrowIfNull(services);
|
|
|
|
var scoped = new HashSet<Type>();
|
|
foreach (var descriptor in services.Where(d => d.Lifetime == ServiceLifetime.Scoped))
|
|
{
|
|
scoped.Add(descriptor.ServiceType);
|
|
|
|
// ImplementationType throws on a keyed descriptor and vice versa, so branch.
|
|
var implementationType = descriptor.IsKeyedService
|
|
? descriptor.KeyedImplementationType
|
|
: descriptor.ImplementationType;
|
|
if (implementationType is not null)
|
|
{
|
|
scoped.Add(implementationType);
|
|
}
|
|
}
|
|
|
|
return scoped;
|
|
}
|
|
}
|