92fe5acca7
AuditLogIngestActor and SiteCallAuditActor each exposed constructors taking a scoped
repository and caching it on a field for the actor's lifetime. Safe only because no
production call site picked those overloads — i.e. one call site away from re-introducing
the row-45 captive-dependency bug — which is why CaptiveDependencyGuard had to allowlist
both fields.
Preferred fix applied: the test fixtures fold onto the IServiceProvider overload with a
stub provider, and the constructors are gone.
Product deletions:
- AuditLogIngestActor: the (IAuditLogRepository, ILogger) ctor, the _injectedRepository
field and its branch in OnIngestAsync, and OnCachedTelemetryWithoutDualWriteAsync (the
"this ctor cannot dual-write" fallback handler that only that ctor installed).
_serviceProvider is now non-nullable and the null-forgiving operators are gone.
- SiteCallAuditActor: both test ctors (repo-only, and repo + reconciliation
collaborators), the _injectedRepository field and its four branches, and the private
SerializedRepository class. _backgroundTimersEnabled went with them: only the deleted
repo-only ctor ever set it false, so it was a constant and its two guards are removed.
The reconciliation tick is still gated — on the collaborators being resolvable, which
is how production reaches the same state.
Test fixtures (12 call sites): two small TestSupport helpers build the stub providers.
SiteCallAuditActor's production wiring resolves a FRESH repository per scope, so the
serializing decorator existed only to protect fixtures that share one DbContext-backed
instance across every scope — it moves into the test project as
SerializingSiteCallAuditRepository, which is where the hazard is actually created.
Fixtures that relied on it keep it. The relay test's ICentralAuditWriter now arrives via
the provider, exactly as production resolves it.
Behaviour is preserved on the paths under test: the deleted audit ctor passed
redactor:null / failureCounter:null, and a provider registering neither resolves both to
null, so the SafeDefaultAuditRedactor fallback and counter-free logging are unchanged. No
fixture on the folded sites sends IngestCachedTelemetryCommand, so swapping to the real
dual-write handler is inert. The SiteCallAudit purge timer now arms in fixtures that
previously had timers off, but every one of them uses default purge settings whose first
tick is 5 minutes out — nothing fires inside a test.
One reconciliation test was renamed and rewritten rather than deleted:
TestCtor_RepositoryOnly_DoesNotStartReconciliationTick became
NoReconciliationCollaboratorsRegistered_DoesNotStartReconciliationTick — the same
guarantee, expressed the way production actually reaches it.
Guard: the allowlist is now EMPTY. The staleness self-test (every entry must still match a
flagged field) is replaced by two stronger pins — Allowlist_IsEmpty asserts nothing is
exempt at all, and FormerlyAllowlistedActors_NowScanClean scans both actors with the
allowlist DISABLED, so the empty allowlist is evidence the fields are gone rather than
evidence the scan stopped looking.
Negative control: re-adding an ISiteCallAuditRepository field to SiteCallAuditActor fails
both FormerlyAllowlistedActors_NowScanClean and the real
Central_NoActorHoldsCaptiveScopedState scan.
Verification: solution builds clean. AuditLog.Tests 3/3 (383), SiteCallAudit.Tests 3/3
(66), Host.Tests 2/2 (505), IntegrationTests 2/2 (115), Communication.Tests 2/2 (717).
285 lines
13 KiB
C#
285 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>
|
|
/// EMPTY, and pinned empty by <c>CaptiveDependencyGuardTests.Allowlist_IsEmpty</c>.
|
|
/// </para>
|
|
/// <para>
|
|
/// It previously held two entries — <c>AuditLogIngestActor._injectedRepository</c> and
|
|
/// <c>SiteCallAuditActor._injectedRepository</c> — nullable repository fields assigned only
|
|
/// by explicitly test-mode constructors. They were safe solely because no production call
|
|
/// site picked those overloads, which left the row-45 captive-dependency bug one
|
|
/// constructor call away. Register row 61 deleted the constructors instead: both actors now
|
|
/// have a single <see cref="IServiceProvider"/> constructor, the fields are gone, and the
|
|
/// test fixtures build stub providers that hand a repository out of a scope. With the shape
|
|
/// removed there is nothing left to exempt.
|
|
/// </para>
|
|
/// <para>
|
|
/// Keep it empty. A new entry means someone reintroduced a scoped service cached on an
|
|
/// actor field; prefer fixing the actor.
|
|
/// </para>
|
|
/// </remarks>
|
|
internal static readonly IReadOnlySet<string> Allowlist =
|
|
new HashSet<string>(StringComparer.Ordinal);
|
|
|
|
/// <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;
|
|
}
|
|
}
|