dohertj2/scadalink-design
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(site-event-logging): resolve SiteEventLogging-006,009,011 — severity index, accurate XML doc, dead-placeholder removal

Browse Source
This commit is contained in:
Joseph Doherty
2026-05-16 22:32:30 -04:00
parent a3d359fff7
commit b1ea78a9fd
5 changed files with 128 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

49
code-reviews/SiteEventLogging/findings.md
View File

@@ -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.