dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(auditlog+sitecallaudit): delete the _injectedRepository test ctors — register row 61

Browse Source 
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).
This commit is contained in:
Joseph Doherty
2026-08-15 17:49:39 -04:00
parent 7a7d345b89
commit 92fe5acca7
15 changed files with 474 additions and 452 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CaptiveDependencyGuard.cs
+15 -18
View File
@@ -72,28 +72,25 @@ internal static class CaptiveDependencyGuard
/// </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.
/// EMPTY, and pinned empty by <c>CaptiveDependencyGuardTests.Allowlist_IsEmpty</c>.
/// </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.
/// 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)
{
"ZB.MOM.WW.ScadaBridge.AuditLog.Central.AuditLogIngestActor._injectedRepository",
"ZB.MOM.WW.ScadaBridge.SiteCallAudit.SiteCallAuditActor._injectedRepository",
};
internal static readonly IReadOnlySet<string> Allowlist =
new HashSet<string>(StringComparer.Ordinal);
/// <summary>
/// Scans the given actor types for instance fields holding captive scope state.
tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CaptiveDependencyGuardTests.cs
+53 -26
View File
@@ -166,38 +166,65 @@ public class CaptiveDependencyGuardSelfTests
}
/// <summary>
/// The allowlist is an escape hatch, so it is itself pinned: every entry must name a
/// field that still exists and would still be flagged. An entry that has stopped matching
/// is either a stale exemption or — worse — a typo silently covering nothing.
/// Register row 61: the allowlist is now EMPTY and must stay that way.
///
/// <para>
/// This replaces the former staleness self-test, which checked that each entry still named
/// a field the scanner would flag. Its two entries — <c>AuditLogIngestActor</c> and
/// <c>SiteCallAuditActor</c>'s <c>_injectedRepository</c> — covered repository fields
/// assigned only by test-mode constructors; those constructors have been deleted and the
/// fixtures fold onto the <c>IServiceProvider</c> overload, so there is no longer a field
/// to exempt. An empty allowlist is a strictly stronger statement than a correct one:
/// nothing is exempt, so nothing can rot.
/// </para>
/// </summary>
[Fact]
public void Allowlist_EntriesAllStillMatchARealFlaggedField()
public void Allowlist_IsEmpty_NoActorExemptFromTheCaptiveDependencyScan()
{
var actorTypes = CaptiveDependencyGuard.ProductionActorTypes();
// Scan with the allowlist disabled against a scoped set wide enough to include the
// repositories the allowlisted fields hold.
var everyScopedType = CaptiveDependencyGuard.ScopedTypes(BuildCentralishScopedProbe());
var unfiltered = CaptiveDependencyGuard.Scan(actorTypes, everyScopedType, applyAllowlist: false);
var flagged = unfiltered
.Select(v => $"{v.ActorType.FullName}.{v.FieldName}")
.ToHashSet(StringComparer.Ordinal);
foreach (var entry in CaptiveDependencyGuard.Allowlist)
{
Assert.True(
flagged.Contains(entry),
$"Allowlist entry '{entry}' no longer matches a flagged field. Remove it — a stale " +
"exemption is indistinguishable from an exemption that never worked.");
}
Assert.True(
CaptiveDependencyGuard.Allowlist.Count == 0,
"The captive-dependency allowlist must stay empty (register row 61). Entries present: " +
string.Join(", ", CaptiveDependencyGuard.Allowlist) +
". An entry means an actor caches a scoped service on a field — fix the actor " +
"(fold test fixtures onto the IServiceProvider overload) rather than exempting it.");
}
/// <summary>
/// A minimal descriptor set naming the two repository types the allowlisted fields hold,
/// so <see cref="Allowlist_EntriesAllStillMatchARealFlaggedField"/> does not need to boot a
/// host. Both are registered <c>Scoped</c> by <c>AddConfigurationDatabase</c>; the
/// role-scan tests below verify that against the real container.
/// Register row 61 (the other half of the pin): with the allowlist empty, the two actors
/// that used to need exemptions must now scan CLEAN against the scoped repository types
/// they resolve. This is what makes the empty allowlist meaningful — it proves the entries
/// were removed because the fields are gone, not because the scan stopped looking.
/// </summary>
[Fact]
public void FormerlyAllowlistedActors_NowScanClean()
{
var actorTypes = CaptiveDependencyGuard.ProductionActorTypes();
var everyScopedType = CaptiveDependencyGuard.ScopedTypes(BuildCentralishScopedProbe());
// Allowlist disabled, so a surviving _injectedRepository-shaped field would still show.
var unfiltered = CaptiveDependencyGuard.Scan(actorTypes, everyScopedType, applyAllowlist: false);
var offenders = unfiltered
.Where(v => v.ActorType.Name is "AuditLogIngestActor" or "SiteCallAuditActor")
.Select(v => v.ToString())
.ToList();
Assert.True(offenders.Count == 0,
"AuditLogIngestActor / SiteCallAuditActor must hold no scoped repository field " +
"(register row 61). Found: " + string.Join(" | ", offenders));
// Sanity: the scan really did examine those two actors, so the clean result above is
// evidence rather than an empty walk.
Assert.Contains(actorTypes, t => t.Name == "AuditLogIngestActor");
Assert.Contains(actorTypes, t => t.Name == "SiteCallAuditActor");
}
/// <summary>
/// A minimal descriptor set naming the two repository types the deleted test-mode
/// constructors used to cache, so <see cref="FormerlyAllowlistedActors_NowScanClean"/> does
/// not need to boot a host. Both are registered <c>Scoped</c> by
/// <c>AddConfigurationDatabase</c>; the role-scan tests below verify that against the real
/// container.
/// </summary>
/// <returns>The probe descriptors.</returns>
private static IServiceCollection BuildCentralishScopedProbe()