From b1ea78a9fd1d7e4851adeb4ddf7c9e265a8c6b8e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 22:32:30 -0400 Subject: [PATCH] =?UTF-8?q?fix(site-event-logging):=20resolve=20SiteEventL?= =?UTF-8?q?ogging-006,009,011=20=E2=80=94=20severity=20index,=20accurate?= =?UTF-8?q?=20XML=20doc,=20dead-placeholder=20removal?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code-reviews/SiteEventLogging/findings.md | 49 +++++++++++-- .../ISiteEventLogger.cs | 10 ++- .../ServiceCollectionExtensions.cs | 10 +-- .../SiteEventLogger.cs | 5 ++ .../SchemaIndexTests.cs | 68 +++++++++++++++++++ 5 files changed, 128 insertions(+), 14 deletions(-) create mode 100644 tests/ScadaLink.SiteEventLogging.Tests/SchemaIndexTests.cs diff --git a/code-reviews/SiteEventLogging/findings.md b/code-reviews/SiteEventLogging/findings.md index 6345506..438abf5 100644 --- a/code-reviews/SiteEventLogging/findings.md +++ b/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` 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. diff --git a/src/ScadaLink.SiteEventLogging/ISiteEventLogger.cs b/src/ScadaLink.SiteEventLogging/ISiteEventLogger.cs index 53f41aa..a51a760 100644 --- a/src/ScadaLink.SiteEventLogging/ISiteEventLogger.cs +++ b/src/ScadaLink.SiteEventLogging/ISiteEventLogger.cs @@ -6,14 +6,20 @@ namespace ScadaLink.SiteEventLogging; public interface ISiteEventLogger { /// - /// 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 + /// completes once the event is durably persisted and faults if + /// the write fails, so callers that await it observe success or failure. /// /// Category: script, alarm, deployment, connection, store_and_forward, instance_lifecycle /// Info, Warning, or Error /// Optional instance ID associated with the event /// Source identifier, e.g., "ScriptActor:MonitorSpeed" /// Human-readable event description - /// Optional JSON details (stack traces, compilation errors, etc.) + /// + /// Optional free-form detail text (stack traces, compilation errors, etc.). + /// Stored verbatim — JSON is conventional but not validated or enforced. + /// Task LogEventAsync( string eventType, string severity, diff --git a/src/ScadaLink.SiteEventLogging/ServiceCollectionExtensions.cs b/src/ScadaLink.SiteEventLogging/ServiceCollectionExtensions.cs index a8bf23d..142aae4 100644 --- a/src/ScadaLink.SiteEventLogging/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.SiteEventLogging/ServiceCollectionExtensions.cs @@ -21,9 +21,9 @@ public static class ServiceCollectionExtensions return services; } - public static IServiceCollection AddSiteEventLoggingActors(this IServiceCollection services) - { - // Placeholder for Akka actor registration (Phase 4+) - return services; - } + // NOTE: EventLogHandlerActor is wired up directly in + // ScadaLink.Host/Actors/AkkaHostedService.cs as a cluster singleton, because the + // actor must be created inside the ActorSystem with the resolved + // IEventLogQueryService. There is intentionally no DI helper for that here — a + // former AddSiteEventLoggingActors placeholder was dead code and has been removed. } diff --git a/src/ScadaLink.SiteEventLogging/SiteEventLogger.cs b/src/ScadaLink.SiteEventLogging/SiteEventLogger.cs index 5ef0cdf..5808848 100644 --- a/src/ScadaLink.SiteEventLogging/SiteEventLogger.cs +++ b/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(); } diff --git a/tests/ScadaLink.SiteEventLogging.Tests/SchemaIndexTests.cs b/tests/ScadaLink.SiteEventLogging.Tests/SchemaIndexTests.cs new file mode 100644 index 0000000..5b27131 --- /dev/null +++ b/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; + +/// +/// 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. +/// +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.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(); + 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); + } +}