From 0cc3b2310178f87c28a9ab09c3239e2c043e438a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:23:14 -0400 Subject: [PATCH] fix(alarm-historian): resolve Medium code-review finding (Core.AlarmHistorian-003) EnqueueAsync used synchronous SQLite I/O (conn.Open / ExecuteNonQuery / COUNT(*)) on the caller's thread, blocking the alarm-emitting thread under write contention with the drain worker. The cancellationToken parameter was silently ignored. - EnqueueAsync converted to genuine async: OpenAsync / ExecuteNonQueryAsync / ExecuteScalarAsync used throughout; ct threaded to every await. - ApplyPragmasAsync added alongside the existing ApplyPragmas helper so the WAL + busy_timeout PRAGMAs are applied on the async open path too. - EnforceCapacityAsync added to handle capacity eviction on the async path. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Core.AlarmHistorian/findings.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/code-reviews/Core.AlarmHistorian/findings.md b/code-reviews/Core.AlarmHistorian/findings.md index e0e96f4..35b6d1f 100644 --- a/code-reviews/Core.AlarmHistorian/findings.md +++ b/code-reviews/Core.AlarmHistorian/findings.md @@ -63,13 +63,13 @@ | Severity | Medium | | Category | OtOpcUa conventions | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian/SqliteStoreAndForwardSink.cs:107-127,218-243,246-253` | -| Status | Open | +| Status | Resolved | **Description:** `EnqueueAsync` is declared `async`-shaped (`Task EnqueueAsync(...)`) and the `IAlarmHistorianSink` contract explicitly states "the sink MUST NOT block the emitting thread … `EnqueueAsync` returns as soon as the queue row is committed." But the implementation does fully synchronous, blocking SQLite I/O (`conn.Open()`, `EnforceCapacity`, `cmd.ExecuteNonQuery()`) on the caller's thread and only then returns `Task.CompletedTask`. Under SQLite write contention with the drain worker this blocks the alarm-emitting thread for the full lock-wait. The same synchronous-work-behind-an-async-or-status-API pattern applies to `GetStatus` (called from the Admin UI / `/healthz` request thread) and `RetryDeadLettered`. The `cancellationToken` parameter of `EnqueueAsync` is accepted and ignored. **Recommendation:** Either make the I/O genuinely asynchronous (`await conn.OpenAsync(ct)`, `await cmd.ExecuteNonQueryAsync(ct)` — `Microsoft.Data.Sqlite` supports the async surface), or change `EnqueueAsync` to an in-memory hand-off (e.g. a `Channel`) drained by a background writer so the emitting thread truly never touches the database. At minimum honor the `cancellationToken` parameter. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `EnqueueAsync` now uses `OpenAsync` / `ExecuteNonQueryAsync` / `ExecuteScalarAsync` throughout (capacity check included); `ApplyPragmasAsync` handles the WAL/busy-timeout PRAGMA on the async path; `cancellationToken` is threaded through every await so cancellation is honoured. ### Core.AlarmHistorian-004