docs(code-reviews): regenerate index after batch 4 low/medium fixes

code-reviews/README.md

@@ -42,8 +42,8 @@ module file and counted in **Total**.
 | Critical | 0 |
 | High | 0 |
 | Medium | 4 |
-| Low | 23 |
+| Low | 4 |
-| **Total** | **27** |
+| **Total** | **8** |
 
 ## Module Status
 
@@ -64,10 +64,10 @@ module file and counted in **Total**.
 | [ManagementService](ManagementService/findings.md) | 2026-05-16 | `9c60592` | 0/0/0/0 | 0 | 13 |
 | [NotificationService](NotificationService/findings.md) | 2026-05-16 | `9c60592` | 0/0/0/0 | 0 | 13 |
 | [Security](Security/findings.md) | 2026-05-16 | `9c60592` | 0/0/0/0 | 0 | 11 |
-| [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-16 | `9c60592` | 0/0/0/3 | 3 | 11 |
+| [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-16 | `9c60592` | 0/0/0/0 | 0 | 11 |
-| [SiteRuntime](SiteRuntime/findings.md) | 2026-05-16 | `9c60592` | 0/0/0/5 | 5 | 16 |
+| [SiteRuntime](SiteRuntime/findings.md) | 2026-05-16 | `9c60592` | 0/0/0/0 | 0 | 16 |
-| [StoreAndForward](StoreAndForward/findings.md) | 2026-05-16 | `9c60592` | 0/0/0/7 | 7 | 14 |
+| [StoreAndForward](StoreAndForward/findings.md) | 2026-05-16 | `9c60592` | 0/0/0/0 | 0 | 14 |
-| [TemplateEngine](TemplateEngine/findings.md) | 2026-05-16 | `9c60592` | 0/0/0/4 | 4 | 14 |
+| [TemplateEngine](TemplateEngine/findings.md) | 2026-05-16 | `9c60592` | 0/0/0/0 | 0 | 14 |
 
 ## Pending Findings
 
@@ -93,7 +93,7 @@ _None open._
 | Host-002 | [Host](Host/findings.md) | Akka.Persistence required by REQ-HOST-6 is not configured and not used |
 | InboundAPI-007 | [InboundAPI](InboundAPI/findings.md) | `Database.Connection()` script API from the design doc is not implemented |
 
-### Low (23)
+### Low (4)
 
 | ID | Module | Title |
 |----|--------|-------|
@@ -101,22 +101,3 @@ _None open._
 | DeploymentManager-013 | [DeploymentManager](DeploymentManager/findings.md) | SMTP credentials serialized and broadcast to all sites |
 | ExternalSystemGateway-011 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | Every call performs a full repository scan of all systems and methods |
 | InboundAPI-012 | [InboundAPI](InboundAPI/findings.md) | `ParameterDefinition` POCO declared in the component project, not Commons |
-| SiteEventLogging-006 | [SiteEventLogging](SiteEventLogging/findings.md) | Missing indexes for severity and keyword-search query paths |
-| SiteEventLogging-009 | [SiteEventLogging](SiteEventLogging/findings.md) | XML doc on `LogEventAsync` claims asynchronous behaviour |
-| SiteEventLogging-011 | [SiteEventLogging](SiteEventLogging/findings.md) | Stale "Phase 4+" placeholder in `ServiceCollectionExtensions` |
-| SiteRuntime-012 | [SiteRuntime](SiteRuntime/findings.md) | `AttributeAccessor`/`ScopeAccessors` block the script on a synchronous Ask |
-| SiteRuntime-013 | [SiteRuntime](SiteRuntime/findings.md) | `HandleUnsubscribeDebugView` does nothing despite documented behaviour |
-| SiteRuntime-014 | [SiteRuntime](SiteRuntime/findings.md) | Trigger-expression evaluation blocks the coordinator actor thread |
-| SiteRuntime-015 | [SiteRuntime](SiteRuntime/findings.md) | `LoggerFactory` created per Instance Actor and never disposed |
-| SiteRuntime-016 | [SiteRuntime](SiteRuntime/findings.md) | Short-lived execution actors, replication actor, and repositories are untested |
-| StoreAndForward-002 | [StoreAndForward](StoreAndForward/findings.md) | Messages enqueued with no registered handler are buffered but never deliverable |
-| StoreAndForward-006 | [StoreAndForward](StoreAndForward/findings.md) | `GetParkedMessagesAsync` count and page run without a transaction |
-| StoreAndForward-007 | [StoreAndForward](StoreAndForward/findings.md) | Async work in `ParkedMessageHandlerActor` uses `ContinueWith` without scheduler/affinity guarantees |
-| StoreAndForward-008 | [StoreAndForward](StoreAndForward/findings.md) | A SQLite connection is opened and torn down on every storage call |
-| StoreAndForward-009 | [StoreAndForward](StoreAndForward/findings.md) | `OnActivity` event invocation is not thread-safe against concurrent subscribe/unsubscribe |
-| StoreAndForward-011 | [StoreAndForward](StoreAndForward/findings.md) | `StoreAndForwardMessageStatus.InFlight` is unused and the doc's "retrying" status is unmodelled |
-| StoreAndForward-012 | [StoreAndForward](StoreAndForward/findings.md) | `StoreAndForwardMessage` is a persistence entity but lives in the component, not Commons |
-| TemplateEngine-011 | [TemplateEngine](TemplateEngine/findings.md) | `SortedPropertiesConverterFactory` is dead code with a misleading comment |
-| TemplateEngine-012 | [TemplateEngine](TemplateEngine/findings.md) | `DataType` enum naming diverges from the design doc |
-| TemplateEngine-013 | [TemplateEngine](TemplateEngine/findings.md) | `ToDictionary(t => t.Id)` throws on duplicate IDs; cycle detectors overload Id 0 as a sentinel |
-| TemplateEngine-014 | [TemplateEngine](TemplateEngine/findings.md) | Template-deletion constraint logic is duplicated and divergent |

code-reviews/SiteEventLogging/findings.md

@@ -8,7 +8,7 @@
 | Last reviewed | 2026-05-16 |
 | Reviewer | claude-agent |
 | Commit reviewed | `9c60592` |
-| Open findings | 3 |
+| Open findings | 0 |
 
 ## Summary
 
@@ -284,7 +284,7 @@ caller returns in <500 ms while the database is held busy) plus
 |--|--|
 | Severity | Low |
 | Category | Performance & resource management |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.SiteEventLogging/SiteEventLogger.cs:50-52`, `src/ScadaLink.SiteEventLogging/EventLogQueryService.cs:65-81` |
 
 **Description**
@@ -306,7 +306,14 @@ cost.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit `pending`): `InitializeSchema` now creates
+`idx_events_severity` on `site_events(severity)`, so severity-filtered queries use an
+index instead of full-scanning. The leading-wildcard `LIKE` keyword search cannot use
+a B-tree index by construction; rather than adding an FTS5 table, that scan is
+accepted and now documented with an inline comment next to the index DDL. Regression
+tests `Schema_HasIndexOnSeverity` and `SeverityFilteredQuery_UsesIndex_NotFullScan`
+(the latter asserts the SQLite query plan uses `idx_events_severity` and does not
+`SCAN site_events`).
 
 ### SiteEventLogging-007 — `ISiteEventLogger` consumers downcast to the concrete type
 
@@ -397,7 +404,7 @@ Regression test `LogEventAsync_FaultsTask_AndCountsFailure_OnWriteError`.
 |--|--|
 | Severity | Low |
 | Category | Documentation & comments |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.SiteEventLogging/ISiteEventLogger.cs:8-10`, `src/ScadaLink.SiteEventLogging/SiteEventLogger.cs:57` |
 
 **Description**
@@ -409,6 +416,16 @@ reasonably assume the write is offloaded and the caller's thread is not blocked,
 which is false. The `details` parameter doc says "Optional JSON details" but nothing
 validates or requires JSON, so callers may pass arbitrary text.
 
+**Re-triage note (2026-05-16)**
+
+The finding's central premise is now stale. SiteEventLogging-005 was resolved by
+introducing a dedicated background writer: `LogEventAsync` enqueues onto an unbounded
+`Channel<T>` and returns without blocking, so the method is now *genuinely*
+asynchronous and the name `LogEventAsync` is accurate. No rename is warranted. The
+only residual documentation defect was the imprecise XML doc (the one-line summary
+did not describe the enqueue/await semantics) and the `details` doc claiming "JSON"
+when nothing enforces it. Scope corrected to those doc fixes only.
+
 **Recommendation**
 
 Align the name, signature, and documentation with the actual behaviour — either make
@@ -417,7 +434,16 @@ Clarify that `details` is free-form text unless JSON is actually enforced.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit `pending`): the `ISiteEventLogger.LogEventAsync` XML doc
+now describes the actual behaviour — the call enqueues onto a background writer and
+returns without blocking, and the returned `Task` completes on durable persistence
+(or faults on write failure). The `details` parameter doc was corrected to "Optional
+free-form detail text ... JSON is conventional but not validated or enforced". No
+rename was needed: post-SiteEventLogging-005 the method is genuinely asynchronous, so
+`LogEventAsync` is accurate (see Re-triage note). Documentation-only change; no
+regression test added (behaviour is already covered by the SiteEventLogging-005
+tests `LogEventAsync_DoesNotBlockCaller_WhenWriteIsSlow` and
+`LogEventAsync_TaskCompletes_AfterEventIsPersisted`).
 
 ### SiteEventLogging-010 — Test coverage gaps: actor bridge, purge/write concurrency, vacuum effectiveness, query error path
 
@@ -473,7 +499,7 @@ SiteEventLogging-001/-002/-003 were resolved
 |--|--|
 | Severity | Low |
 | Category | Documentation & comments |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.SiteEventLogging/ServiceCollectionExtensions.cs:18-22` |
 
 **Description**
@@ -493,4 +519,13 @@ the misleading comment.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit `pending`): confirmed dead code — a repo-wide search
+found zero callers of `AddSiteEventLoggingActors`, and `EventLogHandlerActor` is in
+fact wired up directly in `ScadaLink.Host/Actors/AkkaHostedService.cs` as a cluster
+singleton (it must be created inside the `ActorSystem` with a resolved
+`IEventLogQueryService`, which a `IServiceCollection` extension cannot do). The empty
+placeholder method and its stale "Phase 4+" comment were deleted, and a short
+explanatory note added to `AddSiteEventLogging` pointing readers to where the actor
+is actually registered. Documentation/dead-code change only; no regression test was
+added — the change is a method removal verified by the compiler (no callers) and the
+full module suite still passing.

code-reviews/SiteRuntime/findings.md

@@ -8,7 +8,7 @@
 | Last reviewed | 2026-05-16 |
 | Reviewer | claude-agent |
 | Commit reviewed | `9c60592` |
-| Open findings | 5 |
+| Open findings | 0 |
 
 ## Summary
 
@@ -521,7 +521,7 @@ literal/identifier non-detection, allowed-exception resolution); all 39 existing
 |--|--|
 | Severity | Low |
 | Category | Concurrency & thread safety |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.SiteRuntime/Scripts/ScopeAccessors.cs:28` |
 
 **Description**
@@ -543,7 +543,18 @@ internal-only and exposing only the async API.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (`pending commit`): re-triaged against the current source — the
+finding's own recommendation states the blocking is *acceptable* once SiteRuntime-009's
+dedicated script dispatcher exists, and SiteRuntime-009 is now Resolved
+(`ScriptExecutionActor`/`AlarmExecutionActor` run script bodies on the dedicated
+`ScriptExecutionScheduler` threads, confirmed in `ScriptExecutionActor.cs:74`). A
+blocked accessor therefore can no longer starve the shared `ThreadPool` or Akka
+dispatchers — only a dedicated script thread. The remaining defect was the misleading
+class XML comment, which only said "Reads block on the actor Ask" with no thread-model
+context. The `AttributeAccessor` XML doc now documents the dispatcher containment
+(SiteRuntime-009) explicitly and still steers authors to the async `GetAsync`/`SetAsync`
+variants. No behavioural change — this is a documentation finding; existing
+`ScopeAccessorTests` continue to pass.
 
 ### SiteRuntime-013 — `HandleUnsubscribeDebugView` does nothing despite documented behaviour
 
@@ -551,7 +562,7 @@ _Unresolved._
 |--|--|
 | Severity | Low |
 | Category | Documentation & comments |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.SiteRuntime/Actors/InstanceActor.cs:414` |
 
 **Description**
@@ -573,7 +584,18 @@ comment to state explicitly that subscription teardown is handled by
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (`pending commit`): root cause confirmed — the Instance Actor
+holds no per-subscriber state, so `HandleUnsubscribeDebugView` genuinely has nothing to
+remove; the real debug-stream subscription lifecycle lives in `SiteStreamManager`
+(Subscribe/Unsubscribe/RemoveSubscriber). The recommendation's "correct the XML comment"
+option was taken (removing the handler would still leave `UnsubscribeDebugViewRequest`
+routed here from `DeploymentManagerActor.RouteDebugViewUnsubscribe`, and the no-op
+acknowledgement is harmless). The XML doc on `HandleUnsubscribeDebugView` now states
+explicitly that it is a deliberate no-op acknowledgement and that teardown is handled by
+`SiteStreamManager`; the log message likewise notes "(no-op; subscription teardown
+handled by SiteStreamManager)". This is a documentation-only finding with no observable
+behaviour to regression-test, so no new test was added; the existing
+`InstanceActor`/debug-view tests continue to pass.
 
 ### SiteRuntime-014 — Trigger-expression evaluation blocks the coordinator actor thread
 
@@ -581,7 +603,7 @@ _Unresolved._
 |--|--|
 | Severity | Low |
 | Category | Akka.NET conventions |
-| Status | Open |
+| Status | Deferred |
 | Location | `src/ScadaLink.SiteRuntime/Actors/ScriptActor.cs:219`, `src/ScadaLink.SiteRuntime/Actors/AlarmActor.cs:389` |
 
 **Description**
@@ -605,7 +627,26 @@ without the Roslyn scripting `RunAsync` machinery.
 
 **Resolution**
 
-_Unresolved._
+Deferred 2026-05-16 (`pending commit`): root cause confirmed — `EvaluateExpressionTrigger`
+(ScriptActor) and `EvaluateExpression` (AlarmActor) call
+`_compiledTriggerExpression.RunAsync(...).GetAwaiter().GetResult()` directly inside the
+`AttributeValueChanged` handler, on the coordinator actor's default (thread-pool-backed)
+dispatcher, blocking the mailbox for up to the 2 s timeout. Re-triaged from Open to
+**Deferred** rather than fixed: neither recommended fix stays cleanly in-module without
+a design decision. (a) **Off-thread eval + pipe-back** changes the actor's concurrency
+model — the evaluation carries edge-tracking state (`_lastExpressionResult`) and a
+mutable `_attributeSnapshot`; multiple `AttributeValueChanged` messages can arrive while
+an evaluation is in flight, so a correct fix must decide overlapping-evaluation
+semantics (coalesce / serialize / drop) and the snapshot-coherence contract — a behaviour
+change to the trigger model. (b) **Pre-compile to a plain delegate** would require
+changing the compilation contract: the trigger expression is produced as a Roslyn
+`Script<object?>` by `ScriptCompilationService.CompileTriggerExpression`, which is also
+the security boundary (SiteRuntime-011 trust validation); swapping the artifact type is
+a cross-component change touching the Template Engine / Deployment Manager compile
+pipeline. Given Low severity, a bounded 2 s worst case, and the inline note that trigger
+expressions are trusted, compile-checked, and expected to be cheap, this is left
+Deferred pending a design decision on trigger-evaluation concurrency rather than forcing
+an out-of-scope or messaging-contract-changing fix.
 
 ### SiteRuntime-015 — `LoggerFactory` created per Instance Actor and never disposed
 
@@ -613,7 +654,7 @@ _Unresolved._
 |--|--|
 | Severity | Low |
 | Category | Performance & resource management |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.SiteRuntime/Actors/DeploymentManagerActor.cs:746` |
 
 **Description**
@@ -634,7 +675,18 @@ up per child. Do not create a fresh `LoggerFactory` in a hot creation path.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (`pending commit`): root cause confirmed — `CreateInstanceActor`
+did `new LoggerFactory()` per Instance Actor, never disposed, and detached from the
+host's logging providers. `DeploymentManagerActor` now holds a single `_loggerFactory`
+field, resolved once in the constructor from (in order) a new optional `ILoggerFactory`
+constructor parameter, the injected `IServiceProvider`, or `NullLoggerFactory.Instance`
+as a last resort — never a per-instance allocation. `CreateInstanceActor` mints the
+`InstanceActor` logger from this shared factory, so loggers are routed through the
+application's configured providers and no factory leaks. Regression test:
+`DeploymentManagerLoggerFactoryTests.CreateInstanceActor_ReusesInjectedLoggerFactory_ForEveryInstance`
+injects a counting `ILoggerFactory` and asserts it is used once per created Instance
+Actor — confirmed to fail (0 calls) against the pre-fix `new LoggerFactory()` code and
+pass after the fix.
 
 ### SiteRuntime-016 — Short-lived execution actors, replication actor, and repositories are untested
 
@@ -642,7 +694,7 @@ _Unresolved._
 |--|--|
 | Severity | Low |
 | Category | Testing coverage |
-| Status | Open |
+| Status | Resolved |
 | Location | `tests/ScadaLink.SiteRuntime.Tests/` |
 
 **Description**
@@ -666,4 +718,18 @@ synthetic-ID lookup, missing-row behaviour).
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (`pending commit`): re-triaged against the current test sources —
+`SiteExternalSystemRepository` and `SiteNotificationRepository` are already covered by
+`Repositories/SiteRepositoryTests.cs` (added when SiteRuntime-006/007 were resolved:
+round-trip read and synthetic-ID-stable-across-restart). The execution-actor gap is now
+closed by a new `Actors/ExecutionActorTests.cs` — six tests covering
+`ScriptExecutionActor` (success → `ScriptCallResult` reply + PoisonPill self-stop;
+script-throws → failure reply + stop; cooperative timeout → failure reply + stop;
+no-`replyTo` fire-and-forget still self-stops) and `AlarmExecutionActor` (success →
+self-stop; on-trigger throws → still self-stops). `SiteReplicationActor` is *not* covered
+here: it depends on `Cluster.Get(Context.System)` and so requires a clustered
+`ActorSystem` HOCON harness that does not yet exist in this test project — adding that
+harness is a larger test-infrastructure task tracked separately and out of scope for a
+Low-severity coverage finding; the highest-value untested paths the finding called out
+(script timeout/failure/reply/self-stop) are now covered. Full module suite: 192 tests
+green.

code-reviews/StoreAndForward/findings.md

@@ -8,7 +8,7 @@
 | Last reviewed | 2026-05-16 |
 | Reviewer | claude-agent |
 | Commit reviewed | `9c60592` |
-| Open findings | 7 |
+| Open findings | 0 (3 Deferred: 002, 011, 012 — see notes) |
 
 ## Summary
 
@@ -97,7 +97,7 @@ commit whose message references `StoreAndForward-001`.
 | Severity | Low |
 | Original severity | High (re-triaged down to Low on 2026-05-16 — see Re-triage note) |
 | Category | Error handling & resilience |
-| Status | Open |
+| Status | Deferred |
 | Location | `src/ScadaLink.StoreAndForward/StoreAndForwardService.cs:162`, `:201` |
 
 **Description**
@@ -152,9 +152,17 @@ should be made deliberately rather than forced here.
 
 **Resolution**
 
-_Open — re-triaged to Low. Premise (no handler registration anywhere) is stale: Host
+_Deferred 2026-05-16 (re-triaged High → Low). Verified again against the source in this
-now wires all three handlers. Residual gap is minor and the prescribed fix is a
+pass: the finding's premise (no `RegisterDeliveryHandler` caller anywhere) is stale —
-cross-module contract change needing a design decision._
+`ScadaLink.Host` wires all three handlers at site startup — so the High-severity
+"engine cannot deliver anything" outcome no longer occurs. The residual gap (a message
+enqueued for a category that genuinely has no handler is buffered then skipped forever)
+is real but minor. The prescribed fix — making `EnqueueAsync` reject when no handler is
+registered — is a behavioural contract change that depends on whether late handler
+registration is supported and requires updating tests in NotificationService and
+ExternalSystemGateway (modules outside this review's edit scope). That is a deliberate
+cross-module design decision, not a localised in-module bug fix, so it is **Deferred**
+pending that decision rather than forced here._
 
 ### StoreAndForward-003 — Off-by-one in retry accounting: immediate failure pre-counts as retry 1
 
@@ -319,7 +327,7 @@ other writer's `RetryCount`).
 |--|--|
 | Severity | Low |
 | Category | Performance & resource management |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.StoreAndForward/StoreAndForwardStorage.cs:166`, `:175` |
 
 **Description**
@@ -339,7 +347,22 @@ removes the inconsistency.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending). Confirmed the root cause against the source —
+`GetParkedMessagesAsync` issued `COUNT(*)` then a paged `SELECT` as two separate
+commands on the same connection with no surrounding transaction, so a write committed
+between them yields a `TotalCount` inconsistent with the page. Applied the
+recommendation's preferred option: both reads now run inside a single
+`SqliteTransaction` (`BeginTransactionAsync`), and `CommitAsync` is called after the
+page is read; SQLite's deferred read transaction freezes a consistent snapshot on the
+first read so the count and page agree. Regression test
+`GetParkedMessagesAsync_TransactionedReads_CountMatchesFullResultSet` is a functional
+guard that the transaction wiring did not break pagination (reported `TotalCount`
+agrees with the rows assembled across all pages). Note: a true red-then-green TDD test
+of the *race itself* is not achievable deterministically — reproducing it requires a
+concurrent writer to commit in the sub-millisecond window between the two adjacent
+`SELECT`s; a concurrent stress harness passed even against the pre-fix code, so it
+would not be a real regression test. The fix is nonetheless correct and matches the
+finding's recommendation.
 
 ### StoreAndForward-007 — Async work in `ParkedMessageHandlerActor` uses `ContinueWith` without scheduler/affinity guarantees
 
@@ -347,7 +370,7 @@ _Unresolved._
 |--|--|
 | Severity | Low |
 | Category | Akka.NET conventions |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.StoreAndForward/ParkedMessageHandlerActor.cs:34`, `:68`, `:87` |
 
 **Description**
@@ -370,7 +393,21 @@ off the actor thread safely, and makes the success/failure branches explicit.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending). Confirmed the root cause: all three handlers
+(`HandleQuery`, `HandleRetry`, `HandleDiscard`) used `ContinueWith(...).PipeTo(sender)`
+with an `IsCompletedSuccessfully` check standing in for explicit success/failure
+branches. Applied the recommendation exactly — each now uses
+`PipeTo(sender, success: ..., failure: ...)`, the documented Akka pattern: the success
+projection builds the normal response, the failure projection builds the error
+response, and a faulted antecedent unambiguously routes to `failure` rather than
+relying on an `IsCompletedSuccessfully` convention. `Sender` is still captured into a
+local before the await, and the projections touch only locals. This is a
+behaviour-preserving refactor; the existing `ParkedMessageHandlerActorTests` (8 tests
+covering Query/Retry/Discard request-to-response mapping, correlation-ID propagation
+and the unknown-message responses) act as the regression suite and all pass. No new
+test was added because the observable behaviour is unchanged and the `failure`
+projection cannot be exercised without a service that throws — `StoreAndForwardService`
+is a concrete non-virtual type with no failure-injection seam.
 
 ### StoreAndForward-008 — A SQLite connection is opened and torn down on every storage call
 
@@ -378,7 +415,7 @@ _Unresolved._
 |--|--|
 | Severity | Low |
 | Category | Performance & resource management |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.StoreAndForward/StoreAndForwardStorage.cs:28`, `:61`, `:93`, `:117`, `:144`, `:162`, `:199`, `:221`, `:237`, `:267`, `:285`, `:305`, `:319` |
 
 **Description**
@@ -398,7 +435,18 @@ the design relies on the Sqlite connection pool for acceptable performance.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending). Confirmed the finding's analysis is accurate but
+correctly classified as Low/not-a-correctness-bug: Microsoft.Data.Sqlite pools
+connections, so the per-call `OpenAsync` reuses a pooled handle. Applied the "at
+minimum" remedy from the recommendation — the `StoreAndForwardStorage` class XML
+documentation now explicitly records that the connection-per-call style is a deliberate
+trade-off, that the retry sweep's acceptable performance relies on the
+Microsoft.Data.Sqlite connection pool, and that the remedy if profiling ever shows the
+pooled open to be a hot-path bottleneck is a batched sweep API opening one connection
+and transaction per sweep. The larger batched-API refactor was not undertaken because
+it is not warranted at Low severity and the documented design intent removes the
+"silent reliance on the pool" concern. Documentation-only change — no behavioural code
+touched, so no regression test (the connection-pool reliance is not test-observable).
 
 ### StoreAndForward-009 — `OnActivity` event invocation is not thread-safe against concurrent subscribe/unsubscribe
 
@@ -406,7 +454,7 @@ _Unresolved._
 |--|--|
 | Severity | Low |
 | Category | Concurrency & thread safety |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.StoreAndForward/StoreAndForwardService.cs:46`, `:309` |
 
 **Description**
@@ -430,7 +478,21 @@ notifications asynchronously.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending). Confirmed — and found the impact is worse than
+the finding states. `RaiseActivity` previously did `OnActivity?.Invoke(...)`; a
+throwing subscriber's exception escaped it. On the `EnqueueAsync` immediate-success
+path the `RaiseActivity("Delivered", ...)` call sits *inside* the delivery `try`, so a
+throwing subscriber was caught by the transient-failure handler — a successfully
+delivered message was then buffered, and because the catch block's own
+`RaiseActivity("Queued", ...)` also threw, the exception escaped `EnqueueAsync`
+entirely. `RaiseActivity` now snapshots `OnActivity`, iterates its invocation list, and
+wraps each subscriber call in `try/catch` (logging and ignoring a fault) — activity
+logging is best-effort and a slow/throwing subscriber can neither abort the caller nor
+be misclassified as a delivery failure. Regression tests:
+`EnqueueAsync_ImmediateDeliverySuccess_FaultingActivitySubscriber_StillReportsDelivered`
+(failed pre-fix — the subscriber exception escaped and the call threw; passes post-fix
+with `WasBuffered == false` and an empty buffer) and
+`RetryMessageAsync_FaultingActivitySubscriber_DoesNotIncrementRetryCount`.
 
 ### StoreAndForward-010 — Retry of a parked message does not reset `LastAttemptAt`, so its retry timing is unspecified
 
@@ -479,7 +541,7 @@ cleared, message excluded from the retry-due set) and passes post-fix.
 |--|--|
 | Severity | Low |
 | Category | Design-document adherence |
-| Status | Open |
+| Status | Deferred |
 | Location | `src/ScadaLink.Commons/Types/Enums/StoreAndForwardMessageStatus.cs:9`; `src/ScadaLink.StoreAndForward/StoreAndForwardService.cs:219`, `:235` |
 
 **Description**
@@ -500,7 +562,18 @@ sweep is actively delivering (which would also help with finding 005).
 
 **Resolution**
 
-_Unresolved._
+_Deferred 2026-05-16. Confirmed against the source: `StoreAndForwardMessageStatus`
+defines `Pending, InFlight, Parked, Delivered`; a codebase-wide search shows the
+StoreAndForward module only ever assigns `Pending` and `Parked`, and `InFlight` /
+`Delivered` are never assigned anywhere (delivered messages are deleted, not marked).
+The design doc's `retrying` state is unmodelled. Both options the recommendation offers
+— (a) drop the unused `InFlight`/`Delivered` members, or (b) add a `Retrying` member —
+require editing `StoreAndForwardMessageStatus.cs`, which lives in `src/ScadaLink.Commons`
+(outside this review's edit scope: only `src/ScadaLink.StoreAndForward/**` may be
+changed). The enum is also referenced by IntegrationTests and HealthMonitoring tests, so
+removing members is a cross-module change. The defect is real but cannot be resolved
+in-module; **Deferred** to a change that owns the Commons enum and the design doc
+together._
 
 ### StoreAndForward-012 — `StoreAndForwardMessage` is a persistence entity but lives in the component, not Commons
 
@@ -508,7 +581,7 @@ _Unresolved._
 |--|--|
 | Severity | Low |
 | Category | Code organization & conventions |
-| Status | Open |
+| Status | Deferred |
 | Location | `src/ScadaLink.StoreAndForward/StoreAndForwardMessage.cs:9` |
 
 **Description**
@@ -532,7 +605,19 @@ local persistence model. Document the decision.
 
 **Resolution**
 
-_Unresolved._
+_Deferred 2026-05-16. Confirmed: `StoreAndForwardMessage` is a persistence-ignorant POCO
+mapping to `sf_messages` and is also carried across Akka remoting inside
+`ReplicationOperation`, so it doubles as a de-facto wire contract while living in the
+component assembly rather than the Commons `Entities`/`Messages` hierarchy. The
+recommendation's primary remedy — moving `StoreAndForwardMessage` (and
+`ReplicationOperation`) into Commons — crosses module boundaries (it would add a type to
+`src/ScadaLink.Commons`, outside this review's edit scope of
+`src/ScadaLink.StoreAndForward/**`, and change every referencing module). The alternative
+"separate replication DTO" still leaves the persistence entity in the component, so it
+does not actually resolve the finding's core concern (entity placement / contract-
+evolution governance). Resolving this is a deliberate code-organisation decision that
+must own the Commons hierarchy; **Deferred** rather than forced in-module. Flagged for a
+cross-module follow-up._
 
 ### StoreAndForward-013 — Critical paths lack test coverage: retry-due timing, replication-from-active, and the actor bridge
 

code-reviews/TemplateEngine/findings.md

@@ -8,7 +8,7 @@
 | Last reviewed | 2026-05-16 |
 | Reviewer | claude-agent |
 | Commit reviewed | `9c60592` |
-| Open findings | 4 |
+| Open findings | 0 |
 
 ## Summary
 
@@ -474,7 +474,7 @@ behaviour change; no regression test (documentation-only).
 |--|--|
 | Severity | Low |
 | Category | Documentation & comments |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.TemplateEngine/Flattening/RevisionHashService.cs:136` |
 
 **Description**
@@ -497,7 +497,18 @@ ordering of the `Hashable*` records (ideally enforced by a test).
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit `pending commit`): deleted the dead
+`SortedPropertiesConverterFactory` (and removed it from `CanonicalJsonOptions`),
+and replaced the misleading "sorted keys / consistent ordering" comments with an
+explicit DETERMINISM CONTRACT note on the `RevisionHashService` class summary —
+`System.Text.Json` serializes properties in CLR declaration order and does not
+sort, so stable hashes rely on the private `Hashable*` records declaring their
+properties alphabetically (collections are explicitly sorted by `CanonicalName`).
+That manual ordering is now guarded by a regression test:
+`RevisionHashServiceTests.HashableRecords_PropertiesDeclaredAlphabetically`
+(reflects over the nested `Hashable*` types and asserts ordinal-alphabetical
+property declaration order), so adding a property out of order now fails the build's
+test gate instead of silently changing every revision hash.
 
 ### TemplateEngine-012 — `DataType` enum naming diverges from the design doc
 
@@ -505,7 +516,7 @@ _Unresolved._
 |--|--|
 | Severity | Low |
 | Category | Design-document adherence |
-| Status | Open |
+| Status | Deferred |
 | Location | `src/ScadaLink.TemplateEngine/Validation/SemanticValidator.cs:18` |
 
 **Description**
@@ -523,9 +534,30 @@ are numeric.
 Update `docs/requirements/Component-TemplateEngine.md` to list the actual enum members,
 or rename the enum to match the doc if "Integer" is the intended canonical name.
 
+**Re-triage**
+
+Verified against the source: the `DataType` enum is declared in
+`src/ScadaLink.Commons/Types/Enums/DataType.cs` (`Boolean, Int32, Float, Double,
+String, DateTime, Binary`) — **not** in the TemplateEngine module — and is consumed
+across modules (`TemplateAttribute` entity, management command contracts). The only
+in-module file the finding cites, `SemanticValidator.cs:18`, is confirmed **correct**:
+`NumericDataTypes` already hard-codes the real enum names. Both remediation options
+in the recommendation therefore land **outside** this module's resolution boundary
+(`src/ScadaLink.TemplateEngine/**`): renaming the enum touches `ScadaLink.Commons`
+(and every consumer of `DataType`), and the alternative — updating the design doc —
+touches `docs/requirements/Component-TemplateEngine.md`. There is no in-module code
+defect to fix. Re-triaged from Open to Deferred: the fix is a one-line design-doc
+correction (list the actual seven enum members instead of "Boolean, Integer, Float,
+String") that must be made by an agent owning the docs / Commons scope.
+
 **Resolution**
 
-_Unresolved._
+Deferred 2026-05-16 (no commit): no in-module fix possible — see Re-triage. The
+TemplateEngine code is correct as-is. FLAGGED for the docs owner: correct the
+Attribute data-type list in `docs/requirements/Component-TemplateEngine.md` to match
+`ScadaLink.Commons` `DataType` (`Boolean, Int32, Float, Double, String, DateTime,
+Binary`). Renaming the enum is not recommended (cross-module churn for no behavioural
+gain); the doc is the authoritative thing to fix.
 
 ### TemplateEngine-013 — `ToDictionary(t => t.Id)` throws on duplicate IDs; cycle detectors overload Id 0 as a sentinel
 
@@ -533,7 +565,7 @@ _Unresolved._
 |--|--|
 | Severity | Low |
 | Category | Correctness & logic bugs |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.TemplateEngine/CycleDetector.cs:30`, `src/ScadaLink.TemplateEngine/CycleDetector.cs:38` |
 
 **Description**
@@ -556,7 +588,23 @@ special.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit `pending commit`): added `CycleDetector.BuildLookup`,
+a duplicate-tolerant Id-keyed lookup (`Dictionary` + `TryAdd`, first occurrence wins)
+that replaces every `allTemplates.ToDictionary(t => t.Id)` in the static helpers —
+`CycleDetector` (all three methods), `TemplateResolver.ResolveAllMembers`, and
+`CollisionDetector.DetectCollisions` — so a list containing two templates with the
+same `Id` (e.g. not-yet-saved templates carrying `Id 0`) no longer throws an
+unhandled `ArgumentException`. Separately, the `0`-as-"no-parent" sentinel was
+removed: `DetectInheritanceCycle` now walks the parent chain via the `int?`
+`ParentTemplateId` (`HasValue` gates continuation), and `DetectCrossGraphCycle`
+gates the proposed parent/composed edges on `HasValue` rather than `!= 0`, so a
+template with a real `Id` of 0 is treated like any other node and cycles through it
+are detected. Regression tests:
+`CycleDetectorTests.DetectInheritanceCycle_DuplicateIdsInList_DoesNotThrow`,
+`DetectCompositionCycle_DuplicateIdsInList_DoesNotThrow`,
+`DetectCrossGraphCycle_DuplicateIdsInList_DoesNotThrow`,
+`DetectInheritanceCycle_RealIdZero_StillDetectsCycle`,
+`DetectInheritanceCycle_ParentChainThroughIdZero_DetectsCycle`.
 
 ### TemplateEngine-014 — Template-deletion constraint logic is duplicated and divergent
 
@@ -564,7 +612,7 @@ _Unresolved._
 |--|--|
 | Severity | Low |
 | Category | Code organization & conventions |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.TemplateEngine/TemplateService.cs:109`, `src/ScadaLink.TemplateEngine/Services/TemplateDeletionService.cs:27` |
 
 **Description**
@@ -586,4 +634,17 @@ vice versa) so the constraint logic lives in exactly one place.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit `pending commit`): `TemplateService.DeleteTemplateAsync`
+no longer re-implements the deletion-constraint rules — it now delegates the
+constraint check and the delete to `TemplateDeletionService.DeleteTemplateAsync`
+(the surviving single implementation, which already accumulates every blocking
+reason rather than returning on the first failing category). `TemplateService`
+retains only its audit-logging side effect: after a successful delete it writes the
+`Delete` audit entry and calls `SaveChangesAsync` (the deletion service is unaware of
+auditing and persists the delete itself, so the audit entry needs its own save).
+The constraint logic now lives in exactly one place, so a future rule change cannot
+drift between two implementations. Behavioural change: `DeleteTemplateAsync` now
+reports all blocking reasons and uses `TemplateDeletionService`'s phrasing — the
+affected `TemplateServiceTests` delete tests were updated to the unified messages,
+and a regression test `DeleteTemplate_MultipleConstraints_ReportsAllNotJustFirst`
+verifies all three constraint categories are surfaced together.

src/ScadaLink.SiteEventLogging/ISiteEventLogger.cs

@@ -6,14 +6,20 @@ namespace ScadaLink.SiteEventLogging;
 public interface ISiteEventLogger
 {
     /// <summary>
-    /// Record an event asynchronously.
+    /// Record an event asynchronously. The call enqueues the event onto a background
+    /// writer and returns without blocking the caller on disk I/O. The returned
+    /// <see cref="Task"/> completes once the event is durably persisted and faults if
+    /// the write fails, so callers that <c>await</c> it observe success or failure.
     /// </summary>
     /// <param name="eventType">Category: script, alarm, deployment, connection, store_and_forward, instance_lifecycle</param>
     /// <param name="severity">Info, Warning, or Error</param>
     /// <param name="instanceId">Optional instance ID associated with the event</param>
     /// <param name="source">Source identifier, e.g., "ScriptActor:MonitorSpeed"</param>
     /// <param name="message">Human-readable event description</param>
-    /// <param name="details">Optional JSON details (stack traces, compilation errors, etc.)</param>
+    /// <param name="details">
+    /// Optional free-form detail text (stack traces, compilation errors, etc.).
+    /// Stored verbatim — JSON is conventional but not validated or enforced.
+    /// </param>
     Task LogEventAsync(
         string eventType,
         string severity,

src/ScadaLink.SiteEventLogging/ServiceCollectionExtensions.cs

@@ -21,9 +21,9 @@ public static class ServiceCollectionExtensions
         return services;
     }
 
-    public static IServiceCollection AddSiteEventLoggingActors(this IServiceCollection services)
+    // NOTE: EventLogHandlerActor is wired up directly in
-    {
+    // ScadaLink.Host/Actors/AkkaHostedService.cs as a cluster singleton, because the
-        // Placeholder for Akka actor registration (Phase 4+)
+    // actor must be created inside the ActorSystem with the resolved
-        return services;
+    // IEventLogQueryService. There is intentionally no DI helper for that here — a
-    }
+    // former AddSiteEventLoggingActors placeholder was dead code and has been removed.
 }

src/ScadaLink.SiteEventLogging/SiteEventLogger.cs

@@ -123,7 +123,12 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable
             CREATE INDEX IF NOT EXISTS idx_events_timestamp ON site_events(timestamp);
             CREATE INDEX IF NOT EXISTS idx_events_type ON site_events(event_type);
             CREATE INDEX IF NOT EXISTS idx_events_instance ON site_events(instance_id);
+            CREATE INDEX IF NOT EXISTS idx_events_severity ON site_events(severity);
             """;
+        // The query service also supports keyword search via leading-wildcard
+        // LIKE on message/source. A leading-wildcard LIKE cannot use a B-tree
+        // index, so that path intentionally full-scans; severity/event_type/
+        // instance_id/timestamp filters above are all covered.
         cmd.ExecuteNonQuery();
     }
 

src/ScadaLink.SiteRuntime/Actors/DeploymentManagerActor.cs

@@ -34,6 +34,14 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
     private readonly SiteStreamManager? _streamManager;
     private readonly SiteRuntimeOptions _options;
     private readonly ILogger<DeploymentManagerActor> _logger;
+    /// <summary>
+    /// Shared logger factory used to mint <see cref="InstanceActor"/> loggers
+    /// (SiteRuntime-015). Reused across every <see cref="CreateInstanceActor"/>
+    /// call rather than newing a per-instance factory that is never disposed.
+    /// When the host injects its configured factory the Instance Actor logs are
+    /// routed through the application's logging providers.
+    /// </summary>
+    private readonly ILoggerFactory _loggerFactory;
     private readonly IActorRef? _dclManager;
     private readonly IActorRef? _replicationActor;
     private readonly ISiteHealthCollector? _healthCollector;
@@ -59,7 +67,8 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
         IActorRef? dclManager = null,
         IActorRef? replicationActor = null,
         ISiteHealthCollector? healthCollector = null,
-        IServiceProvider? serviceProvider = null)
+        IServiceProvider? serviceProvider = null,
+        ILoggerFactory? loggerFactory = null)
     {
         _storage = storage;
         _compilationService = compilationService;
@@ -71,6 +80,13 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
         _healthCollector = healthCollector;
         _serviceProvider = serviceProvider;
         _logger = logger;
+        // SiteRuntime-015: reuse a single logger factory for all Instance Actors.
+        // Prefer an explicitly injected factory, fall back to one resolved from
+        // the service provider, and only as a last resort use NullLoggerFactory —
+        // never a per-instance `new LoggerFactory()` that leaks undisposed.
+        _loggerFactory = loggerFactory
+            ?? serviceProvider?.GetService(typeof(ILoggerFactory)) as ILoggerFactory
+            ?? Microsoft.Extensions.Logging.Abstractions.NullLoggerFactory.Instance;
 
         // Lifecycle commands
         Receive<DeployInstanceCommand>(HandleDeploy);
@@ -942,7 +958,8 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
             return;
         }
 
-        var loggerFactory = new LoggerFactory();
+        // SiteRuntime-015: reuse the shared, host-configured logger factory
+        // instead of allocating (and leaking) a fresh LoggerFactory per instance.
         var props = Props.Create(() => new InstanceActor(
             instanceName,
             configJson,
@@ -951,7 +968,7 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
             _sharedScriptLibrary,
             _streamManager,
             _options,
-            loggerFactory.CreateLogger<InstanceActor>(),
+            _loggerFactory.CreateLogger<InstanceActor>(),
             _dclManager,
             _healthCollector,
             _serviceProvider));

src/ScadaLink.SiteRuntime/Actors/InstanceActor.cs

@@ -494,12 +494,19 @@ public class InstanceActor : ReceiveActor
     }
 
     /// <summary>
-    /// WP-25: Debug view unsubscribe — removes subscription.
+    /// WP-25: Debug view unsubscribe (SiteRuntime-013).
+    /// This handler is a deliberate no-op acknowledgement: the Instance Actor holds
+    /// no per-subscriber state. The real debug-stream subscription lifecycle lives in
+    /// <see cref="ScadaLink.SiteRuntime.Streaming.SiteStreamManager"/>
+    /// (Subscribe / Unsubscribe / RemoveSubscriber); the gRPC stream is torn down
+    /// there when the central side cancels the call. Nothing is removed here.
     /// </summary>
     private void HandleUnsubscribeDebugView(UnsubscribeDebugViewRequest request)
     {
+        // No subscription state in the Instance Actor — see the XML doc above.
         _logger.LogDebug(
-            "Debug view unsubscribe for {Instance}, correlationId={Id}",
+            "Debug view unsubscribe for {Instance}, correlationId={Id} " +
+            "(no-op; subscription teardown handled by SiteStreamManager)",
             _instanceUniqueName, request.CorrelationId);
     }
 

src/ScadaLink.SiteRuntime/Scripts/ScopeAccessors.cs

@@ -4,8 +4,18 @@ namespace ScadaLink.SiteRuntime.Scripts;
 /// Scope-aware view onto the instance's attributes, anchored at a path prefix.
 /// <c>Attributes["X"]</c> on the root scope resolves to canonical name "X";
 /// on a composition with prefix "TempSensor" it resolves to "TempSensor.X".
-/// Reads block on the actor Ask; async variants are provided for callers
+///
-/// that prefer to await explicitly.
+/// <para>
+/// Thread-model note (SiteRuntime-012): the indexer get/set block synchronously
+/// on the Instance Actor Ask (and, for data-connected attributes, the DCL
+/// round-trip). This is safe because script bodies execute on the dedicated
+/// <see cref="ScriptExecutionScheduler"/> threads (SiteRuntime-009), not the
+/// shared <see cref="System.Threading.ThreadPool"/> — so a blocked accessor
+/// cannot starve unrelated Akka dispatchers or HTTP request handling. The async
+/// variants (<see cref="GetAsync"/>/<see cref="SetAsync"/>) are still preferred
+/// where the script can await, as they avoid holding a dedicated thread idle for
+/// the duration of each round-trip.
+/// </para>
 /// </summary>
 public class AttributeAccessor
 {

src/ScadaLink.StoreAndForward/ParkedMessageHandlerActor.cs

@@ -31,12 +31,15 @@ public class ParkedMessageHandlerActor : ReceiveActor
         var sender = Sender;
         var siteId = _siteId;
 
+        // StoreAndForward-007: idiomatic PipeTo with explicit success/failure
+        // projections instead of ContinueWith. Both projections touch only locals
+        // (captured before the await), so they are safe to run off the actor thread.
         _service.GetParkedMessagesAsync(category: null, msg.PageNumber, msg.PageSize)
-            .ContinueWith(t =>
+            .PipeTo(
-            {
+                sender,
-                if (t.IsCompletedSuccessfully)
+                success: result =>
                 {
-                    var entries = t.Result.Messages
+                    var entries = result.Messages
                         .Select(m => new ParkedMessageEntry(
                             MessageId: m.Id,
                             TargetSystem: m.Target,
@@ -51,14 +54,12 @@ public class ParkedMessageHandlerActor : ReceiveActor
                         .ToList();
 
                     return new ParkedMessageQueryResponse(
-                        msg.CorrelationId, siteId, entries, t.Result.TotalCount,
+                        msg.CorrelationId, siteId, entries, result.TotalCount,
                         msg.PageNumber, msg.PageSize, true, null, DateTimeOffset.UtcNow);
-                }
+                },
-
+                failure: ex => new ParkedMessageQueryResponse(
-                return new ParkedMessageQueryResponse(
                     msg.CorrelationId, siteId, [], 0, msg.PageNumber, msg.PageSize,
-                    false, t.Exception?.GetBaseException().Message, DateTimeOffset.UtcNow);
+                    false, ex.GetBaseException().Message, DateTimeOffset.UtcNow));
-            }).PipeTo(sender);
     }
 
     private void HandleRetry(ParkedMessageRetryRequest msg)
@@ -66,18 +67,13 @@ public class ParkedMessageHandlerActor : ReceiveActor
         var sender = Sender;
 
         _service.RetryParkedMessageAsync(msg.MessageId)
-            .ContinueWith(t =>
+            .PipeTo(
-            {
+                sender,
-                if (t.IsCompletedSuccessfully)
+                success: retried => new ParkedMessageRetryResponse(
-                {
+                    msg.CorrelationId, retried,
-                    return new ParkedMessageRetryResponse(
+                    retried ? null : "Message not found or no longer parked."),
-                        msg.CorrelationId, t.Result,
+                failure: ex => new ParkedMessageRetryResponse(
-                        t.Result ? null : "Message not found or no longer parked.");
+                    msg.CorrelationId, false, ex.GetBaseException().Message));
-                }
-
-                return new ParkedMessageRetryResponse(
-                    msg.CorrelationId, false, t.Exception?.GetBaseException().Message);
-            }).PipeTo(sender);
     }
 
     private void HandleDiscard(ParkedMessageDiscardRequest msg)
@@ -85,18 +81,13 @@ public class ParkedMessageHandlerActor : ReceiveActor
         var sender = Sender;
 
         _service.DiscardParkedMessageAsync(msg.MessageId)
-            .ContinueWith(t =>
+            .PipeTo(
-            {
+                sender,
-                if (t.IsCompletedSuccessfully)
+                success: discarded => new ParkedMessageDiscardResponse(
-                {
+                    msg.CorrelationId, discarded,
-                    return new ParkedMessageDiscardResponse(
+                    discarded ? null : "Message not found or no longer parked."),
-                        msg.CorrelationId, t.Result,
+                failure: ex => new ParkedMessageDiscardResponse(
-                        t.Result ? null : "Message not found or no longer parked.");
+                    msg.CorrelationId, false, ex.GetBaseException().Message));
-                }
-
-                return new ParkedMessageDiscardResponse(
-                    msg.CorrelationId, false, t.Exception?.GetBaseException().Message);
-            }).PipeTo(sender);
     }
 
     private static string ExtractMethodName(string payloadJson, Commons.Types.Enums.StoreAndForwardCategory category)

src/ScadaLink.StoreAndForward/StoreAndForwardService.cs

@@ -377,9 +377,34 @@ public class StoreAndForwardService
         return await _storage.GetMessageCountByOriginInstanceAsync(instanceName);
     }
 
+    /// <summary>
+    /// WP-14: Raises the S&amp;F activity notification. StoreAndForward-009: the
+    /// delegate is snapshotted (so a concurrent unsubscribe cannot NRE) and every
+    /// subscriber invocation is wrapped so a slow/throwing subscriber (e.g. the site
+    /// event log) cannot abort the caller. Crucially, a subscriber exception raised
+    /// from <see cref="EnqueueAsync"/> or <c>RetryMessageAsync</c> must NOT be
+    /// misclassified as a transient delivery failure — pre-fix it escaped into the
+    /// delivery try/catch and caused a successfully delivered message to be buffered
+    /// (or its retry count to be bumped). Activity logging is best-effort.
+    /// </summary>
     private void RaiseActivity(string action, StoreAndForwardCategory category, string detail)
     {
-        OnActivity?.Invoke(action, category, detail);
+        var handlers = OnActivity;
+        if (handlers == null) return;
+
+        foreach (var handler in handlers.GetInvocationList().Cast<Action<string, StoreAndForwardCategory, string>>())
+        {
+            try
+            {
+                handler(action, category, detail);
+            }
+            catch (Exception ex)
+            {
+                _logger.LogWarning(ex,
+                    "Store-and-forward activity subscriber threw for action {Action}; ignored",
+                    action);
+            }
+        }
     }
 }
 

src/ScadaLink.StoreAndForward/StoreAndForwardStorage.cs

@@ -8,6 +8,19 @@ namespace ScadaLink.StoreAndForward;
 /// WP-9: SQLite persistence layer for store-and-forward messages.
 /// Uses direct Microsoft.Data.Sqlite (not EF Core) for lightweight site-side storage.
 /// No max buffer size per design decision.
+///
+/// StoreAndForward-008: every method opens a fresh <see cref="SqliteConnection"/> for
+/// the duration of the call rather than holding a long-lived connection. This is a
+/// deliberate trade-off, not an oversight: Microsoft.Data.Sqlite maintains an internal
+/// connection pool keyed on the connection string, so <c>OpenAsync</c> on a previously
+/// used connection string reuses a pooled handle instead of performing a real file
+/// open. The retry sweep therefore relies on that pool for acceptable performance —
+/// it calls <see cref="RemoveMessageAsync"/> / <see cref="UpdateMessageIfStatusAsync"/>
+/// once per due message, and with no max buffer size (by design) the buffer can grow
+/// large. The connection-per-call style keeps each method self-contained and
+/// transaction-scoped; if profiling ever shows the pooled open to be a bottleneck on
+/// the hot retry path, the remedy is a batched sweep API that opens one connection (and
+/// one transaction) per sweep.
 /// </summary>
 public class StoreAndForwardStorage
 {
@@ -222,6 +235,12 @@ public class StoreAndForwardStorage
 
     /// <summary>
     /// WP-12: Gets all parked messages, optionally filtered by category, with pagination.
+    ///
+    /// StoreAndForward-006: the COUNT(*) and the paged SELECT run inside a single
+    /// transaction so they observe one consistent snapshot. Without it, a concurrent
+    /// enqueue/park/discard arriving between the two statements yields a TotalCount
+    /// inconsistent with the returned page (flickering totals / off-by-one page math
+    /// in the paginated UI).
     /// </summary>
     public async Task<(List<StoreAndForwardMessage> Messages, int TotalCount)> GetParkedMessagesAsync(
         StoreAndForwardCategory? category = null,
@@ -231,8 +250,11 @@ public class StoreAndForwardStorage
         await using var connection = new SqliteConnection(_connectionString);
         await connection.OpenAsync();
 
+        await using var transaction = (SqliteTransaction)await connection.BeginTransactionAsync();
+
         // Count
         await using var countCmd = connection.CreateCommand();
+        countCmd.Transaction = transaction;
         countCmd.CommandText = category.HasValue
             ? "SELECT COUNT(*) FROM sf_messages WHERE status = @parked AND category = @category"
             : "SELECT COUNT(*) FROM sf_messages WHERE status = @parked";
@@ -242,6 +264,7 @@ public class StoreAndForwardStorage
 
         // Page
         await using var pageCmd = connection.CreateCommand();
+        pageCmd.Transaction = transaction;
         var categoryFilter = category.HasValue ? " AND category = @category" : "";
         pageCmd.CommandText = $@"
             SELECT id, category, target, payload_json, retry_count, max_retries,
@@ -257,6 +280,8 @@ public class StoreAndForwardStorage
         pageCmd.Parameters.AddWithValue("@offset", (pageNumber - 1) * pageSize);
 
         var messages = await ReadMessagesAsync(pageCmd);
+
+        await transaction.CommitAsync();
         return (messages, totalCount);
     }
 

src/ScadaLink.TemplateEngine/CollisionDetector.cs

@@ -27,7 +27,10 @@ public static class CollisionDetector
         Template template,
         IReadOnlyList<Template> allTemplates)
     {
-        var lookup = allTemplates.ToDictionary(t => t.Id);
+        // Duplicate-tolerant lookup (see CycleDetector.BuildLookup): a plain
+        // ToDictionary(t => t.Id) throws if two templates share an Id (e.g.
+        // not-yet-saved templates carrying Id 0).
+        var lookup = CycleDetector.BuildLookup(allTemplates);
         var allMembers = new List<ResolvedMember>();
 
         // Collect direct (top-level) members

src/ScadaLink.TemplateEngine/CycleDetector.cs

@@ -9,6 +9,21 @@ namespace ScadaLink.TemplateEngine;
 /// </summary>
 public static class CycleDetector
 {
+    /// <summary>
+    /// Builds an Id-keyed lookup that tolerates duplicate Ids in the input list
+    /// (e.g. multiple not-yet-saved templates all carrying Id 0). On a duplicate
+    /// the first occurrence wins — graph walks only need one representative node
+    /// per Id, and a real cycle through any duplicate would still be reachable.
+    /// A plain <c>ToDictionary(t =&gt; t.Id)</c> would instead throw ArgumentException.
+    /// </summary>
+    internal static Dictionary<int, Template> BuildLookup(IReadOnlyList<Template> allTemplates)
+    {
+        var lookup = new Dictionary<int, Template>();
+        foreach (var t in allTemplates)
+            lookup.TryAdd(t.Id, t);
+        return lookup;
+    }
+
     /// <summary>
     /// Checks whether setting <paramref name="parentId"/> as the parent of template
     /// <paramref name="templateId"/> would introduce an inheritance cycle.
@@ -27,28 +42,30 @@ public static class CycleDetector
 
         // Walk the inheritance chain from the proposed parent upward.
         // If we arrive back at templateId, there is a cycle.
-        var lookup = allTemplates.ToDictionary(t => t.Id);
+        var lookup = BuildLookup(allTemplates);
         var visited = new HashSet<int> { templateId };
         var chain = new List<string>();
 
         var templateName = lookup.TryGetValue(templateId, out var tmpl) ? tmpl.Name : templateId.ToString();
         chain.Add(templateName);
 
-        var currentId = parentId;
+        // ParentTemplateId is int? — a missing value (not 0) means "no parent",
-        while (currentId != 0)
+        // so a template with a real Id of 0 is walked like any other node.
+        int? currentId = parentId;
+        while (currentId.HasValue)
         {
-            if (!lookup.TryGetValue(currentId, out var current))
+            if (!lookup.TryGetValue(currentId.Value, out var current))
                 break;
 
             chain.Add(current.Name);
 
-            if (visited.Contains(currentId))
+            if (visited.Contains(currentId.Value))
             {
                 return $"Inheritance cycle detected: {string.Join(" -> ", chain)}.";
             }
 
-            visited.Add(currentId);
+            visited.Add(currentId.Value);
-            currentId = current.ParentTemplateId ?? 0;
+            currentId = current.ParentTemplateId;
         }
 
         return null;
@@ -70,7 +87,7 @@ public static class CycleDetector
             return $"Template '{selfName}' cannot compose itself.";
         }
 
-        var lookup = allTemplates.ToDictionary(t => t.Id);
+        var lookup = BuildLookup(allTemplates);
 
         // BFS/DFS from composedTemplateId through all its compositions.
         // If we reach templateId, that's a cycle.
@@ -115,7 +132,7 @@ public static class CycleDetector
         int? proposedComposedTemplateId,
         IReadOnlyList<Template> allTemplates)
     {
-        var lookup = allTemplates.ToDictionary(t => t.Id);
+        var lookup = BuildLookup(allTemplates);
 
         // Build adjacency: for each template, collect all reachable templates
         // via inheritance (parent) and composition edges.
@@ -124,11 +141,12 @@ public static class CycleDetector
         var visited = new HashSet<int>();
         var queue = new Queue<int>();
 
-        // Seed with proposed targets
+        // Seed with proposed targets. A null proposed id means "no edge"; a value
-        if (proposedParentId.HasValue && proposedParentId.Value != 0)
+        // of 0 is a legitimate Id, so only HasValue gates enqueuing.
+        if (proposedParentId.HasValue)
             queue.Enqueue(proposedParentId.Value);
 
-        if (proposedComposedTemplateId.HasValue && proposedComposedTemplateId.Value != 0)
+        if (proposedComposedTemplateId.HasValue)
             queue.Enqueue(proposedComposedTemplateId.Value);
 
         while (queue.Count > 0)
@@ -146,8 +164,8 @@ public static class CycleDetector
             if (!lookup.TryGetValue(currentId, out var current))
                 continue;
 
-            // Follow inheritance edge
+            // Follow inheritance edge (int? — missing value means no parent)
-            if (current.ParentTemplateId.HasValue && current.ParentTemplateId.Value != 0)
+            if (current.ParentTemplateId.HasValue)
                 queue.Enqueue(current.ParentTemplateId.Value);
 
             // Follow composition edges

src/ScadaLink.TemplateEngine/Flattening/RevisionHashService.cs

@@ -8,19 +8,24 @@ namespace ScadaLink.TemplateEngine.Flattening;
 
 /// <summary>
 /// Produces a deterministic SHA-256 hash of a FlattenedConfiguration.
-/// Same content always produces the same hash across runs by using
+/// Same content always produces the same hash across runs.
-/// canonical JSON serialization with sorted keys.
+/// <para>
+/// DETERMINISM CONTRACT: System.Text.Json serializes properties in CLR
+/// declaration order, which it does NOT sort. Stable output therefore relies
+/// on the private <c>Hashable*</c> records below declaring their properties
+/// in alphabetical order. A contributor adding a property out of alphabetical
+/// order would silently change every revision hash; the ordering is guarded
+/// by <c>RevisionHashServiceTests.HashableRecords_PropertiesDeclaredAlphabetically</c>.
+/// Collections are explicitly sorted by <c>CanonicalName</c> before hashing.
+/// </para>
 /// </summary>
 public class RevisionHashService
 {
     private static readonly JsonSerializerOptions CanonicalJsonOptions = new()
     {
-        // Sort properties alphabetically for determinism
         PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
         DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
-        WriteIndented = false,
+        WriteIndented = false
-        // Ensure consistent ordering
-        Converters = { new SortedPropertiesConverterFactory() }
     };
 
     /// <summary>
@@ -84,7 +89,9 @@ public class RevisionHashService
         return $"sha256:{Convert.ToHexStringLower(hashBytes)}";
     }
 
-    // Internal types for deterministic serialization (sorted property names via camelCase + alphabetical)
+    // Internal types for deterministic serialization. Properties MUST be declared
+    // in alphabetical order — System.Text.Json emits them in declaration order and
+    // does not sort. See the DETERMINISM CONTRACT note on the class summary.
     private sealed record HashableConfiguration
     {
         public List<HashableAlarm> Alarms { get; init; } = [];
@@ -128,14 +135,3 @@ public class RevisionHashService
         public string? TriggerType { get; init; }
     }
 }
-
-/// <summary>
-/// A JSON converter factory that ensures properties are serialized in alphabetical order
-/// for deterministic output. Works with record types.
-/// </summary>
-internal class SortedPropertiesConverterFactory : JsonConverterFactory
-{
-    public override bool CanConvert(Type typeToConvert) => false;
-
-    public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options) => null;
-}

src/ScadaLink.TemplateEngine/TemplateResolver.cs

@@ -39,7 +39,10 @@ public static class TemplateResolver
         int templateId,
         IReadOnlyList<Template> allTemplates)
     {
-        var lookup = allTemplates.ToDictionary(t => t.Id);
+        // Duplicate-tolerant lookup (see CycleDetector.BuildLookup): a plain
+        // ToDictionary(t => t.Id) throws if two templates share an Id (e.g.
+        // not-yet-saved templates carrying Id 0).
+        var lookup = CycleDetector.BuildLookup(allTemplates);
         if (!lookup.TryGetValue(templateId, out var template))
             return Array.Empty<ResolvedTemplateMember>();
 

src/ScadaLink.TemplateEngine/TemplateService.cs

@@ -112,59 +112,18 @@ public class TemplateService
         if (template == null)
             return Result<bool>.Failure($"Template with ID {templateId} not found.");
 
-        // Derived templates are owned by their composition row and must be removed
+        // Deletion-constraint logic (instances / child / derived / composing
-        // by deleting the composition (which cascades) — block direct deletion.
+        // templates) lives in exactly one place — TemplateDeletionService — so a
-        if (template.IsDerived)
+        // future rule change cannot drift between two implementations
-            return Result<bool>.Failure(
+        // (TemplateEngine-014). TemplateService owns only the audit-logging side
-                $"Cannot delete template '{template.Name}': it is a derived template. " +
+        // effect, which the deletion service is unaware of.
-                "Remove the owning composition on its parent template instead.");
+        var deletionService = new Services.TemplateDeletionService(_repository);
+        var deleteResult = await deletionService.DeleteTemplateAsync(templateId, cancellationToken);
+        if (deleteResult.IsFailure)
+            return deleteResult;
 
-        // Check for instances referencing this template
+        // TemplateDeletionService already persisted the delete; the audit entry
-        var instances = await _repository.GetInstancesByTemplateIdAsync(templateId, cancellationToken);
+        // is added to the change tracker here and needs its own SaveChangesAsync.
-        if (instances.Count > 0)
-            return Result<bool>.Failure(
-                $"Cannot delete template '{template.Name}': it is referenced by {instances.Count} instance(s).");
-
-        // Check for child templates inheriting from this template.
-        // Split derived vs. regular children — the message and remediation differ.
-        var allTemplates = await _repository.GetAllTemplatesAsync(cancellationToken);
-        var inheritors = allTemplates.Where(t => t.ParentTemplateId == templateId).ToList();
-        var derivatives = inheritors.Where(t => t.IsDerived).ToList();
-        var regularChildren = inheritors.Where(t => !t.IsDerived).ToList();
-
-        if (regularChildren.Count > 0)
-            return Result<bool>.Failure(
-                $"Cannot delete template '{template.Name}': it is inherited by {regularChildren.Count} child template(s): " +
-                string.Join(", ", regularChildren.Select(c => $"'{c.Name}'")));
-
-        if (derivatives.Count > 0)
-        {
-            // Name each derivative by its owning parent template + composition slot.
-            var ownerCompIds = derivatives.Select(d => d.OwnerCompositionId).Where(id => id.HasValue).Select(id => id!.Value).ToHashSet();
-            var ownerLookup = allTemplates
-                .SelectMany(t => t.Compositions.Select(c => new { Owner = t, Composition = c }))
-                .Where(x => ownerCompIds.Contains(x.Composition.Id))
-                .ToDictionary(x => x.Composition.Id, x => $"'{x.Owner.Name}' (as '{x.Composition.InstanceName}')");
-
-            var details = derivatives
-                .Select(d => d.OwnerCompositionId.HasValue && ownerLookup.TryGetValue(d.OwnerCompositionId.Value, out var label)
-                    ? label
-                    : $"'{d.Name}'");
-            return Result<bool>.Failure(
-                $"Cannot delete template '{template.Name}': it is the base of {derivatives.Count} derived template(s) used in: " +
-                string.Join(", ", details) + ". Remove those compositions first.");
-        }
-
-        // Check for templates composing this template
-        var composedBy = allTemplates
-            .Where(t => t.Compositions.Any(c => c.ComposedTemplateId == templateId))
-            .ToList();
-        if (composedBy.Count > 0)
-            return Result<bool>.Failure(
-                $"Cannot delete template '{template.Name}': it is composed by {composedBy.Count} template(s): " +
-                string.Join(", ", composedBy.Select(c => $"'{c.Name}'")));
-
-        await _repository.DeleteTemplateAsync(templateId, cancellationToken);
         await _auditService.LogAsync(user, "Delete", "Template", templateId.ToString(), template.Name, null, cancellationToken);
         await _repository.SaveChangesAsync(cancellationToken);
 

tests/ScadaLink.SiteEventLogging.Tests/SchemaIndexTests.cs

@@ -0,0 +1,68 @@
+using Microsoft.Data.Sqlite;
+using Microsoft.Extensions.Logging.Abstractions;
+using Microsoft.Extensions.Options;
+
+namespace ScadaLink.SiteEventLogging.Tests;
+
+/// <summary>
+/// Regression tests for SiteEventLogging-006: the schema must index the columns the
+/// query service filters on so common queries do not full-scan a 1 GB database.
+/// </summary>
+public class SchemaIndexTests : IDisposable
+{
+    private readonly SiteEventLogger _logger;
+    private readonly SqliteConnection _verifyConnection;
+    private readonly string _dbPath;
+
+    public SchemaIndexTests()
+    {
+        _dbPath = Path.Combine(Path.GetTempPath(), $"test_index_{Guid.NewGuid()}.db");
+        var options = Options.Create(new SiteEventLogOptions { DatabasePath = _dbPath });
+        _logger = new SiteEventLogger(options, NullLogger<SiteEventLogger>.Instance);
+
+        _verifyConnection = new SqliteConnection($"Data Source={_dbPath}");
+        _verifyConnection.Open();
+    }
+
+    public void Dispose()
+    {
+        _verifyConnection.Dispose();
+        _logger.Dispose();
+        if (File.Exists(_dbPath)) File.Delete(_dbPath);
+    }
+
+    [Fact]
+    public void Schema_HasIndexOnSeverity()
+    {
+        using var cmd = _verifyConnection.CreateCommand();
+        cmd.CommandText =
+            "SELECT name FROM sqlite_master WHERE type = 'index' AND tbl_name = 'site_events'";
+        var indexes = new List<string>();
+        using var reader = cmd.ExecuteReader();
+        while (reader.Read()) indexes.Add(reader.GetString(0));
+
+        Assert.Contains("idx_events_severity", indexes);
+    }
+
+    [Fact]
+    public async Task SeverityFilteredQuery_UsesIndex_NotFullScan()
+    {
+        await _logger.LogEventAsync("script", "Error", null, "S", "boom");
+        await _logger.LogEventAsync("script", "Info", null, "S", "ok");
+
+        using var cmd = _verifyConnection.CreateCommand();
+        cmd.CommandText =
+            "EXPLAIN QUERY PLAN SELECT id FROM site_events WHERE severity = 'Error'";
+        var plan = new System.Text.StringBuilder();
+        using var reader = cmd.ExecuteReader();
+        while (reader.Read())
+        {
+            // The detail column holds the human-readable plan step.
+            plan.Append(reader.GetString(reader.GetOrdinal("detail"))).Append('\n');
+        }
+
+        var planText = plan.ToString();
+        Assert.Contains("idx_events_severity", planText);
+        Assert.DoesNotContain("SCAN site_events", planText);
+    }
+}

tests/ScadaLink.SiteRuntime.Tests/Actors/DeploymentManagerLoggerFactoryTests.cs

@@ -0,0 +1,115 @@
+using Akka.Actor;
+using Akka.TestKit.Xunit2;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Abstractions;
+using ScadaLink.Commons.Messages.Deployment;
+using ScadaLink.Commons.Types.Enums;
+using ScadaLink.Commons.Types.Flattening;
+using ScadaLink.SiteRuntime.Actors;
+using ScadaLink.SiteRuntime.Persistence;
+using ScadaLink.SiteRuntime.Scripts;
+using System.Text.Json;
+
+namespace ScadaLink.SiteRuntime.Tests.Actors;
+
+/// <summary>
+/// Regression test for SiteRuntime-015 — <see cref="DeploymentManagerActor"/> must
+/// reuse a single, injected <see cref="ILoggerFactory"/> for every Instance Actor it
+/// creates rather than newing (and leaking) a fresh <see cref="LoggerFactory"/> per
+/// instance.
+/// </summary>
+public class DeploymentManagerLoggerFactoryTests : TestKit, IDisposable
+{
+    private readonly SiteStorageService _storage;
+    private readonly ScriptCompilationService _compilationService;
+    private readonly SharedScriptLibrary _sharedScriptLibrary;
+    private readonly string _dbFile;
+
+    public DeploymentManagerLoggerFactoryTests()
+    {
+        _dbFile = Path.Combine(Path.GetTempPath(), $"dm-loggerfactory-test-{Guid.NewGuid():N}.db");
+        _storage = new SiteStorageService(
+            $"Data Source={_dbFile}",
+            NullLogger<SiteStorageService>.Instance);
+        _storage.InitializeAsync().GetAwaiter().GetResult();
+        _compilationService = new ScriptCompilationService(
+            NullLogger<ScriptCompilationService>.Instance);
+        _sharedScriptLibrary = new SharedScriptLibrary(
+            _compilationService, NullLogger<SharedScriptLibrary>.Instance);
+    }
+
+    void IDisposable.Dispose()
+    {
+        Shutdown();
+        try { File.Delete(_dbFile); } catch { /* cleanup */ }
+    }
+
+    private static string MakeConfigJson(string instanceName)
+    {
+        var config = new FlattenedConfiguration
+        {
+            InstanceUniqueName = instanceName,
+            Attributes =
+            [
+                new ResolvedAttribute { CanonicalName = "TestAttr", Value = "1", DataType = "Int32" }
+            ]
+        };
+        return JsonSerializer.Serialize(config);
+    }
+
+    /// <summary>
+    /// Counts <see cref="ILoggerFactory.CreateLogger"/> calls and records whether
+    /// the factory was disposed. A passing test proves the single injected factory
+    /// is the one used for every Instance Actor.
+    /// </summary>
+    private sealed class CountingLoggerFactory : ILoggerFactory
+    {
+        public int CreateLoggerCalls;
+        public bool Disposed;
+
+        public ILogger CreateLogger(string categoryName)
+        {
+            Interlocked.Increment(ref CreateLoggerCalls);
+            return NullLogger.Instance;
+        }
+
+        public void AddProvider(ILoggerProvider provider) { }
+        public void Dispose() => Disposed = true;
+    }
+
+    [Fact]
+    public async Task CreateInstanceActor_ReusesInjectedLoggerFactory_ForEveryInstance()
+    {
+        // Pre-populate several enabled instances so startup creates multiple
+        // Instance Actors.
+        const int instanceCount = 6;
+        for (int i = 0; i < instanceCount; i++)
+        {
+            var name = $"Inst{i}";
+            await _storage.StoreDeployedConfigAsync(name, MakeConfigJson(name), $"d{i}", $"h{i}", true);
+        }
+
+        var loggerFactory = new CountingLoggerFactory();
+
+        var actor = ActorOf(Props.Create(() => new DeploymentManagerActor(
+            _storage,
+            _compilationService,
+            _sharedScriptLibrary,
+            null,
+            new SiteRuntimeOptions { StartupBatchSize = 100, StartupBatchDelayMs = 5 },
+            NullLogger<DeploymentManagerActor>.Instance,
+            null,
+            null,
+            null,
+            null,
+            loggerFactory)));
+
+        // Allow async startup (load configs + staggered creation).
+        await Task.Delay(2000);
+
+        // Every Instance Actor logger must come from the single injected factory.
+        // Before the fix, each CreateInstanceActor allocated its own LoggerFactory,
+        // so the injected factory would never be touched (CreateLoggerCalls == 0).
+        Assert.Equal(instanceCount, loggerFactory.CreateLoggerCalls);
+    }
+}

tests/ScadaLink.SiteRuntime.Tests/Actors/ExecutionActorTests.cs

@@ -0,0 +1,171 @@
+using Akka.Actor;
+using Akka.TestKit.Xunit2;
+using Microsoft.CodeAnalysis.CSharp.Scripting;
+using Microsoft.CodeAnalysis.Scripting;
+using Microsoft.Extensions.Logging.Abstractions;
+using ScadaLink.Commons.Messages.ScriptExecution;
+using ScadaLink.Commons.Types.Enums;
+using ScadaLink.Commons.Types.Scripts;
+using ScadaLink.SiteRuntime.Actors;
+using ScadaLink.SiteRuntime.Scripts;
+
+namespace ScadaLink.SiteRuntime.Tests.Actors;
+
+/// <summary>
+/// Regression coverage for SiteRuntime-016 — the short-lived execution actors
+/// (<see cref="ScriptExecutionActor"/>, <see cref="AlarmExecutionActor"/>) were
+/// previously untested. Covers success, exception, timeout, Ask-reply, and the
+/// PoisonPill self-stop after completion.
+/// </summary>
+public class ExecutionActorTests : TestKit, IDisposable
+{
+    private readonly SharedScriptLibrary _sharedLibrary;
+    private readonly ScriptCompilationService _compilationService;
+
+    public ExecutionActorTests()
+    {
+        _compilationService = new ScriptCompilationService(
+            NullLogger<ScriptCompilationService>.Instance);
+        _sharedLibrary = new SharedScriptLibrary(
+            _compilationService, NullLogger<SharedScriptLibrary>.Instance);
+    }
+
+    void IDisposable.Dispose() => Shutdown();
+
+    private static Script<object?> CompileScript(string code)
+    {
+        var scriptOptions = ScriptOptions.Default
+            .WithReferences(typeof(object).Assembly, typeof(Enumerable).Assembly)
+            .WithImports("System", "System.Collections.Generic", "System.Linq", "System.Threading.Tasks");
+        var script = CSharpScript.Create<object?>(code, scriptOptions, typeof(ScriptGlobals));
+        script.Compile();
+        return script;
+    }
+
+    private static SiteRuntimeOptions Options(int timeoutSeconds = 30)
+        => new() { MaxScriptCallDepth = 10, ScriptExecutionTimeoutSeconds = timeoutSeconds };
+
+    // ── ScriptExecutionActor ──
+
+    [Fact]
+    public void ScriptExecutionActor_Success_RepliesWithResultAndStops()
+    {
+        var compiled = CompileScript("return 7 * 6;");
+        var replyTo = CreateTestProbe();
+        var instanceActor = CreateTestProbe();
+
+        var exec = ActorOf(Props.Create(() => new ScriptExecutionActor(
+            "Answer", "Inst1", compiled, null, 0,
+            instanceActor.Ref, _sharedLibrary, Options(),
+            replyTo.Ref, "corr-1", NullLogger.Instance,
+            ScriptScope.Root, null, null)));
+
+        Watch(exec);
+
+        var result = replyTo.ExpectMsg<ScriptCallResult>(TimeSpan.FromSeconds(10));
+        Assert.True(result.Success);
+        Assert.Equal("corr-1", result.CorrelationId);
+        Assert.Equal(42, result.ReturnValue);
+
+        // The actor must PoisonPill itself once execution completes.
+        ExpectTerminated(exec, TimeSpan.FromSeconds(5));
+    }
+
+    [Fact]
+    public void ScriptExecutionActor_ScriptThrows_RepliesFailureAndStops()
+    {
+        var compiled = CompileScript("throw new InvalidOperationException(\"boom\");");
+        var replyTo = CreateTestProbe();
+        var instanceActor = CreateTestProbe();
+
+        var exec = ActorOf(Props.Create(() => new ScriptExecutionActor(
+            "Bad", "Inst1", compiled, null, 0,
+            instanceActor.Ref, _sharedLibrary, Options(),
+            replyTo.Ref, "corr-2", NullLogger.Instance,
+            ScriptScope.Root, null, null)));
+
+        Watch(exec);
+
+        var result = replyTo.ExpectMsg<ScriptCallResult>(TimeSpan.FromSeconds(10));
+        Assert.False(result.Success);
+        Assert.Equal("corr-2", result.CorrelationId);
+        Assert.Contains("boom", result.ErrorMessage);
+
+        ExpectTerminated(exec, TimeSpan.FromSeconds(5));
+    }
+
+    [Fact]
+    public void ScriptExecutionActor_Timeout_RepliesFailureAndStops()
+    {
+        // A long busy loop that observes the cancellation token so the
+        // 1-second timeout fires cooperatively.
+        var compiled = CompileScript(
+            "while (true) { await System.Threading.Tasks.Task.Delay(50, CancellationToken); }");
+        var replyTo = CreateTestProbe();
+        var instanceActor = CreateTestProbe();
+
+        var exec = ActorOf(Props.Create(() => new ScriptExecutionActor(
+            "Slow", "Inst1", compiled, null, 0,
+            instanceActor.Ref, _sharedLibrary, Options(timeoutSeconds: 1),
+            replyTo.Ref, "corr-3", NullLogger.Instance,
+            ScriptScope.Root, null, null)));
+
+        Watch(exec);
+
+        var result = replyTo.ExpectMsg<ScriptCallResult>(TimeSpan.FromSeconds(10));
+        Assert.False(result.Success);
+        Assert.Contains("timed out", result.ErrorMessage);
+
+        ExpectTerminated(exec, TimeSpan.FromSeconds(5));
+    }
+
+    [Fact]
+    public void ScriptExecutionActor_NoReplyTo_StillStopsAfterCompletion()
+    {
+        var compiled = CompileScript("return 1;");
+        var instanceActor = CreateTestProbe();
+
+        // ActorRefs.Nobody as replyTo — fire-and-forget execution.
+        var exec = ActorOf(Props.Create(() => new ScriptExecutionActor(
+            "FireForget", "Inst1", compiled, null, 0,
+            instanceActor.Ref, _sharedLibrary, Options(),
+            ActorRefs.Nobody, "corr-4", NullLogger.Instance,
+            ScriptScope.Root, null, null)));
+
+        Watch(exec);
+        ExpectTerminated(exec, TimeSpan.FromSeconds(5));
+    }
+
+    // ── AlarmExecutionActor ──
+
+    [Fact]
+    public void AlarmExecutionActor_Success_StopsAfterCompletion()
+    {
+        var compiled = CompileScript("return 0;");
+        var instanceActor = CreateTestProbe();
+
+        var exec = ActorOf(Props.Create(() => new AlarmExecutionActor(
+            "HiTemp", "Inst1", AlarmLevel.High, 5, "High temperature",
+            compiled, instanceActor.Ref, _sharedLibrary, Options(),
+            NullLogger.Instance)));
+
+        Watch(exec);
+        ExpectTerminated(exec, TimeSpan.FromSeconds(5));
+    }
+
+    [Fact]
+    public void AlarmExecutionActor_ScriptThrows_StillStops()
+    {
+        var compiled = CompileScript("throw new System.Exception(\"alarm-boom\");");
+        var instanceActor = CreateTestProbe();
+
+        var exec = ActorOf(Props.Create(() => new AlarmExecutionActor(
+            "HiTemp", "Inst1", AlarmLevel.High, 5, "High temperature",
+            compiled, instanceActor.Ref, _sharedLibrary, Options(),
+            NullLogger.Instance)));
+
+        Watch(exec);
+        // Even on a throwing on-trigger body, the actor must self-stop.
+        ExpectTerminated(exec, TimeSpan.FromSeconds(5));
+    }
+}

tests/ScadaLink.StoreAndForward.Tests/StoreAndForwardServiceTests.cs

@@ -372,6 +372,50 @@ public class StoreAndForwardServiceTests : IAsyncLifetime, IDisposable
         Assert.Contains("Queued", activities);
     }
 
+    // ── StoreAndForward-009: faulting activity subscriber must not corrupt delivery ──
+
+    [Fact]
+    public async Task EnqueueAsync_ImmediateDeliverySuccess_FaultingActivitySubscriber_StillReportsDelivered()
+    {
+        // StoreAndForward-009: a throwing OnActivity subscriber (e.g. the site event
+        // log) must not be misclassified as a transient delivery failure. Pre-fix the
+        // subscriber's exception escaped RaiseActivity, was caught by EnqueueAsync's
+        // transient-failure handler, and a successfully delivered message was buffered.
+        _service.OnActivity += (_, _, _) => throw new InvalidOperationException("logging blew up");
+        _service.RegisterDeliveryHandler(StoreAndForwardCategory.ExternalSystem,
+            _ => Task.FromResult(true));
+
+        var result = await _service.EnqueueAsync(
+            StoreAndForwardCategory.ExternalSystem, "api", """{}""");
+
+        Assert.True(result.Accepted);
+        Assert.False(result.WasBuffered); // delivered, NOT buffered
+
+        var msg = await _storage.GetMessageByIdAsync(result.MessageId);
+        Assert.Null(msg); // nothing left in the buffer
+    }
+
+    [Fact]
+    public async Task RetryMessageAsync_FaultingActivitySubscriber_DoesNotIncrementRetryCount()
+    {
+        // StoreAndForward-009: a throwing subscriber raised after a successful retry
+        // delivery must not be caught by the retry-failure handler and counted as a
+        // transient failure.
+        var result = await _service.EnqueueAsync(
+            StoreAndForwardCategory.ExternalSystem, "api", """{}""",
+            attemptImmediateDelivery: false, maxRetries: 5);
+
+        _service.RegisterDeliveryHandler(StoreAndForwardCategory.ExternalSystem,
+            _ => Task.FromResult(true));
+        _service.OnActivity += (_, _, _) => throw new InvalidOperationException("logging blew up");
+
+        await _service.RetryPendingMessagesAsync();
+
+        // The retry succeeded; the message must be gone, not re-buffered with a bumped count.
+        var msg = await _storage.GetMessageByIdAsync(result.MessageId);
+        Assert.Null(msg);
+    }
+
     // ── WP-10: Per-source-entity retry settings ──
 
     [Fact]

tests/ScadaLink.StoreAndForward.Tests/StoreAndForwardStorageTests.cs

@@ -250,6 +250,39 @@ public class StoreAndForwardStorageTests : IAsyncLifetime, IDisposable
         Assert.Equal(2, page2.Count);
     }
 
+    [Fact]
+    public async Task GetParkedMessagesAsync_TransactionedReads_CountMatchesFullResultSet()
+    {
+        // StoreAndForward-006: the COUNT(*) and paged SELECT now run inside one
+        // transaction so they share a consistent snapshot. This functional check
+        // guards the fix — it verifies the transaction wiring did not break paging:
+        // the reported TotalCount and the rows assembled across all pages agree, and
+        // a page wide enough to hold every parked row contains exactly TotalCount rows.
+        for (int i = 0; i < 25; i++)
+        {
+            var m = CreateMessage($"txn-{i}", StoreAndForwardCategory.ExternalSystem);
+            m.Status = StoreAndForwardMessageStatus.Parked;
+            await _storage.EnqueueAsync(m);
+            await _storage.UpdateMessageAsync(m);
+        }
+
+        var (wholePage, wholeTotal) = await _storage.GetParkedMessagesAsync(pageNumber: 1, pageSize: 1000);
+        Assert.Equal(25, wholeTotal);
+        Assert.Equal(wholeTotal, wholePage.Count);
+
+        var collected = new List<string>();
+        int reportedTotal = -1;
+        for (int page = 1; ; page++)
+        {
+            var (rows, total) = await _storage.GetParkedMessagesAsync(pageNumber: page, pageSize: 7);
+            reportedTotal = total;
+            collected.AddRange(rows.Select(r => r.Id));
+            if (rows.Count < 7) break;
+        }
+        Assert.Equal(reportedTotal, collected.Count);
+        Assert.Equal(25, collected.Distinct().Count());
+    }
+
     [Fact]
     public async Task GetMessageCountByStatusAsync_ReturnsAccurateCount()
     {

tests/ScadaLink.TemplateEngine.Tests/CycleDetectorTests.cs

@@ -153,4 +153,77 @@ public class CycleDetectorTests
 
         Assert.Null(result);
     }
+
+    // ========================================================================
+    // TemplateEngine-013: robustness against duplicate Ids and Id 0
+    // ========================================================================
+
+    [Fact]
+    public void DetectInheritanceCycle_DuplicateIdsInList_DoesNotThrow()
+    {
+        // Two not-yet-saved templates both carry Id == 0. ToDictionary(t => t.Id)
+        // would throw ArgumentException; the detector must tolerate it.
+        var templateA = new Template("A") { Id = 0 };
+        var templateB = new Template("B") { Id = 0 };
+        var saved = new Template("Saved") { Id = 1 };
+        var all = new List<Template> { templateA, templateB, saved };
+
+        var ex = Record.Exception(() => CycleDetector.DetectInheritanceCycle(1, 0, all));
+
+        Assert.Null(ex);
+    }
+
+    [Fact]
+    public void DetectCompositionCycle_DuplicateIdsInList_DoesNotThrow()
+    {
+        var templateA = new Template("A") { Id = 0 };
+        var templateB = new Template("B") { Id = 0 };
+        var all = new List<Template> { templateA, templateB };
+
+        var ex = Record.Exception(() => CycleDetector.DetectCompositionCycle(1, 2, all));
+
+        Assert.Null(ex);
+    }
+
+    [Fact]
+    public void DetectCrossGraphCycle_DuplicateIdsInList_DoesNotThrow()
+    {
+        var templateA = new Template("A") { Id = 0 };
+        var templateB = new Template("B") { Id = 0 };
+        var all = new List<Template> { templateA, templateB };
+
+        var ex = Record.Exception(() => CycleDetector.DetectCrossGraphCycle(5, 1, 2, all));
+
+        Assert.Null(ex);
+    }
+
+    [Fact]
+    public void DetectInheritanceCycle_RealIdZero_StillDetectsCycle()
+    {
+        // A template legitimately stored with Id 0 (in-memory / test scenario):
+        // a self-inheritance attempt must still be detected, not skipped as
+        // "no parent" by a 0-as-sentinel overload.
+        var template = new Template("Zero") { Id = 0 };
+        var all = new List<Template> { template };
+
+        var result = CycleDetector.DetectInheritanceCycle(0, 0, all);
+
+        Assert.NotNull(result);
+        Assert.Contains("itself", result);
+    }
+
+    [Fact]
+    public void DetectInheritanceCycle_ParentChainThroughIdZero_DetectsCycle()
+    {
+        // Child(1) -> parent Zero(0) -> parent Child(1): a cycle running through
+        // a template whose real Id is 0 must be detected, not silently skipped.
+        var zero = new Template("Zero") { Id = 0, ParentTemplateId = 1 };
+        var child = new Template("Child") { Id = 1, ParentTemplateId = null };
+        var all = new List<Template> { zero, child };
+
+        var result = CycleDetector.DetectInheritanceCycle(1, 0, all);
+
+        Assert.NotNull(result);
+        Assert.Contains("cycle", result, StringComparison.OrdinalIgnoreCase);
+    }
 }

tests/ScadaLink.TemplateEngine.Tests/Flattening/RevisionHashServiceTests.cs

@@ -99,6 +99,38 @@ public class RevisionHashServiceTests
         Assert.Equal(_sut.ComputeHash(config1), _sut.ComputeHash(config2));
     }
 
+    [Fact]
+    public void HashableRecords_PropertiesDeclaredAlphabetically()
+    {
+        // TemplateEngine-011: revision-hash determinism depends entirely on the
+        // private Hashable* records declaring their properties in alphabetical
+        // order (System.Text.Json emits properties in CLR declaration order and
+        // does not sort). This guards against a contributor silently changing
+        // every revision hash by adding a property out of order.
+        var nested = typeof(RevisionHashService)
+            .GetNestedTypes(System.Reflection.BindingFlags.NonPublic)
+            .Where(t => t.Name.StartsWith("Hashable"))
+            .ToList();
+
+        Assert.NotEmpty(nested);
+
+        foreach (var type in nested)
+        {
+            var propNames = type
+                .GetProperties(System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance)
+                .Where(p => p.Name != "EqualityContract")
+                .Select(p => p.Name)
+                .ToList();
+
+            var sorted = propNames.OrderBy(n => n, StringComparer.Ordinal).ToList();
+
+            Assert.True(
+                propNames.SequenceEqual(sorted),
+                $"{type.Name} properties must be declared alphabetically. " +
+                $"Declared: [{string.Join(", ", propNames)}] Expected: [{string.Join(", ", sorted)}]");
+        }
+    }
+
     private static FlattenedConfiguration CreateConfig(string instanceName, string tempValue)
     {
         return new FlattenedConfiguration

tests/ScadaLink.TemplateEngine.Tests/TemplateServiceTests.cs

@@ -122,11 +122,13 @@ public class TemplateServiceTests
         _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny<CancellationToken>())).ReturnsAsync(template);
         _repoMock.Setup(r => r.GetInstancesByTemplateIdAsync(1, It.IsAny<CancellationToken>()))
             .ReturnsAsync(new List<Instance> { new Instance("Pump1") { Id = 1, TemplateId = 1, SiteId = 1 } });
+        _repoMock.Setup(r => r.GetAllTemplatesAsync(It.IsAny<CancellationToken>()))
+            .ReturnsAsync(new List<Template> { template });
 
         var result = await _service.DeleteTemplateAsync(1, "admin");
 
         Assert.True(result.IsFailure);
-        Assert.Contains("referenced by", result.Error);
+        Assert.Contains("instance(s) reference it", result.Error);
     }
 
     [Fact]
@@ -143,7 +145,7 @@ public class TemplateServiceTests
         var result = await _service.DeleteTemplateAsync(1, "admin");
 
         Assert.True(result.IsFailure);
-        Assert.Contains("inherited by", result.Error);
+        Assert.Contains("child template(s) inherit from it", result.Error);
     }
 
     [Fact]
@@ -162,7 +164,36 @@ public class TemplateServiceTests
         var result = await _service.DeleteTemplateAsync(1, "admin");
 
         Assert.True(result.IsFailure);
-        Assert.Contains("composed by", result.Error);
+        Assert.Contains("template(s) compose it", result.Error);
+    }
+
+    [Fact]
+    public async Task DeleteTemplate_MultipleConstraints_ReportsAllNotJustFirst()
+    {
+        // TemplateEngine-014: DeleteTemplateAsync delegates its constraint check
+        // to the single TemplateDeletionService implementation, which accumulates
+        // every blocking reason instead of returning on the first failing category.
+        var template = new Template("Busy") { Id = 1 };
+        var composer = new Template("Composer") { Id = 3 };
+        composer.Compositions.Add(new TemplateComposition("Module") { Id = 1, TemplateId = 3, ComposedTemplateId = 1 });
+
+        _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny<CancellationToken>())).ReturnsAsync(template);
+        _repoMock.Setup(r => r.GetInstancesByTemplateIdAsync(1, It.IsAny<CancellationToken>()))
+            .ReturnsAsync(new List<Instance> { new Instance("Inst1") { Id = 1, TemplateId = 1, SiteId = 1 } });
+        _repoMock.Setup(r => r.GetAllTemplatesAsync(It.IsAny<CancellationToken>()))
+            .ReturnsAsync(new List<Template>
+            {
+                template,
+                new Template("Child") { Id = 2, ParentTemplateId = 1 },
+                composer
+            });
+
+        var result = await _service.DeleteTemplateAsync(1, "admin");
+
+        Assert.True(result.IsFailure);
+        Assert.Contains("instance(s) reference it", result.Error);
+        Assert.Contains("child template(s) inherit from it", result.Error);
+        Assert.Contains("template(s) compose it", result.Error);
     }
 
     // ========================================================================