Commit Graph

2 Commits

Author SHA1 Message Date
Joseph Doherty 92fe5acca7 refactor(auditlog+sitecallaudit): delete the _injectedRepository test ctors — register row 61
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).
2026-08-15 17:49:39 -04:00
Joseph Doherty 00a804e6d9 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.
2026-08-15 11:58:40 -04:00