From 0529cf2d403dcfdff0da2ebf4aa1416e313287ee Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 19:47:17 -0400 Subject: [PATCH] =?UTF-8?q?fix(site-event-logging):=20resolve=20SiteEventL?= =?UTF-8?q?ogging-001/002/003,=20re-triage=20004=20=E2=80=94=20incremental?= =?UTF-8?q?=20auto=5Fvacuum,=20cap-purge=20guard,=20write-lock=20connectio?= =?UTF-8?q?n=20access?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code-reviews/SiteEventLogging/findings.md | 72 ++++++- .../EventLogPurgeService.cs | 99 +++++++--- .../EventLogQueryService.cs | 61 +++--- .../SiteEventLogger.cs | 74 ++++++-- .../EventLogPurgeServiceTests.cs | 177 +++++++++++++++++- .../EventLogQueryServiceTests.cs | 27 +-- 6 files changed, 411 insertions(+), 99 deletions(-) diff --git a/code-reviews/SiteEventLogging/findings.md b/code-reviews/SiteEventLogging/findings.md index c92fb07..213356c 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 | 11 | +| Open findings | 7 | ## Summary @@ -51,7 +51,7 @@ the requirement strictly, and several documentation/maintainability issues. |--|--| | Severity | High | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.SiteEventLogging/EventLogPurgeService.cs:100-102`, `src/ScadaLink.SiteEventLogging/SiteEventLogger.cs:36-55` | **Description** @@ -78,7 +78,12 @@ deletes, or measure logical data size (e.g. `page_count - freelist_count` times **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit ``): `InitializeSchema` now sets +`PRAGMA auto_vacuum = INCREMENTAL` before any table is created, and +`GetDatabaseSizeBytes` measures logical size as `(page_count - freelist_count) * +page_size` so reclaimed pages no longer mask the size drop. The cap-purge loop now +reliably observes the database shrinking. Regression test +`PurgeByStorageCap_StopsWhenUnderCap_DoesNotEmptyTable`. ### SiteEventLogging-002 — Storage-cap purge deletes the entire table when space is not reclaimed @@ -86,7 +91,7 @@ _Unresolved._ |--|--| | Severity | High | | Category | Design-document adherence | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.SiteEventLogging/EventLogPurgeService.cs:87-105` | **Description** @@ -111,7 +116,14 @@ removed. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit ``): with the size measurement fixed +(SiteEventLogging-001) the cap loop terminates when the file is genuinely under the +cap. An additional guard stops the loop if the on-disk size fails to decrease across +an iteration, so a cap that can never be met no longer empties the whole table. +Regression tests `PurgeByStorageCap_StopsWhenUnderCap_DoesNotEmptyTable` (asserts the +table is not emptied and the file ends under a realistic non-zero cap) and +`PurgeByStorageCap_RemovesOldestEventsFirst` (asserts only the oldest events are +removed). ### SiteEventLogging-003 — Shared `SqliteConnection` used by purge and query without the write lock @@ -119,7 +131,7 @@ _Unresolved._ |--|--| | Severity | High | | Category | Concurrency & thread safety | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.SiteEventLogging/EventLogPurgeService.cs:64,90,100,110,114`, `src/ScadaLink.SiteEventLogging/EventLogQueryService.cs:36`, `src/ScadaLink.SiteEventLogging/SiteEventLogger.cs:34,72` | **Description** @@ -145,15 +157,24 @@ makes this safer). Do not share one `SqliteConnection` across threads. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit ``): the raw `internal Connection` property was +removed from `SiteEventLogger` and replaced with lock-guarded `WithConnection(...)` +overloads that hold the existing `_writeLock` for the duration of the caller's +delegate. `EventLogPurgeService`, `EventLogQueryService`, and `LogEventAsync` all now +access the connection exclusively through `WithConnection`, so the purge thread, +query thread, and recording threads are serialised on a single lock. `Dispose` was +also brought under the lock to avoid a dispose/use race. Regression test +`PurgeByStorageCap_ConcurrentWritesDoNotCorruptConnection` exercises purge running +concurrently with multiple writer threads. ### SiteEventLogging-004 — Event-log handler runs as a cluster singleton that can land on the standby node | | | |--|--| -| Severity | High | +| Severity | Low | +| Original severity | High (re-triaged down to Low on 2026-05-16 — see Re-triage note) | | Category | Design-document adherence | -| Status | Open | +| Status | Won't Fix | | Location | `src/ScadaLink.Host/Actors/AkkaHostedService.cs:313-336`, `src/ScadaLink.SiteEventLogging/EventLogHandlerActor.cs:21-25` | **Description** @@ -179,9 +200,40 @@ the active node explicitly (the node owning the Deployment Manager singleton), o handler are guaranteed co-located. Reconcile the design doc and the inline comment with whichever model is chosen. +**Re-triage note (2026-05-16)** + +The finding's central claim — that a remote query "can be served by the standby +node and read that node's near-empty database" — is incorrect for the query path. +In `AkkaHostedService.cs` the `event-log-handler` `ClusterSingletonManager` and the +`deployment-manager` `ClusterSingletonManager` are created with the **same role** +(`siteRole`) in the **same cluster**. Akka.NET pins every cluster singleton of a +given role to the *oldest member of that role* — so all same-role singletons in a +cluster co-locate on one node. The "active node" in this codebase is, by definition, +the node hosting the `deployment-manager` singleton; the event-log query singleton +is therefore *guaranteed* to run on that same node. A `ClusterClient` query cannot +land on the standby. The inline comment in `AkkaHostedService.cs` is accurate, not +"the opposite of what happens". + +A real but distinct concern exists: the *writer* (`SiteEventLogger`) is registered +as a plain per-node DI singleton (`AddSiteEventLogging`), so it records to a local +SQLite file on **every** node, including the standby. That wastes storage on the +standby but does **not** cause the query-returns-nothing symptom the finding +describes, because the query singleton always reads the *active* node's (populated) +database. Gating the writer to the active node would be a `ScadaLink.Host` wiring +change, outside this module's scope, and is a minor optimisation rather than a +correctness defect. + +Re-triaged from High to Low and closed as **Won't Fix**: the High-severity +correctness claim does not hold. Any residual cleanup (gate the standby-node writer; +the comment needs no change) can be raised as a fresh Low finding against +`ScadaLink.Host` if desired. + **Resolution** -_Unresolved._ +Won't Fix — 2026-05-16 (commit ``). Re-triaged: the asserted defect (query +served by standby returning an empty log) cannot occur because the event-log query +singleton and the deployment-manager singleton share a role and so always co-locate +on the active node. No code change made; see the re-triage note above. ### SiteEventLogging-005 — `LogEventAsync` performs synchronous disk I/O on the caller's thread diff --git a/src/ScadaLink.SiteEventLogging/EventLogPurgeService.cs b/src/ScadaLink.SiteEventLogging/EventLogPurgeService.cs index 07cd7ed..b7b4251 100644 --- a/src/ScadaLink.SiteEventLogging/EventLogPurgeService.cs +++ b/src/ScadaLink.SiteEventLogging/EventLogPurgeService.cs @@ -1,4 +1,3 @@ -using Microsoft.Data.Sqlite; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -12,6 +11,9 @@ namespace ScadaLink.SiteEventLogging; /// public class EventLogPurgeService : BackgroundService { + /// Number of events deleted per cap-purge batch. + private const int CapPurgeBatchSize = 1000; + private readonly SiteEventLogger _eventLogger; private readonly SiteEventLogOptions _options; private readonly ILogger _logger; @@ -21,7 +23,7 @@ public class EventLogPurgeService : BackgroundService IOptions options, ILogger logger) { - // We need the concrete type to access the connection + // We need the concrete type to funnel access through its shared lock. _eventLogger = (SiteEventLogger)eventLogger; _options = options.Value; _logger = logger; @@ -61,10 +63,13 @@ public class EventLogPurgeService : BackgroundService { var cutoff = DateTimeOffset.UtcNow.AddDays(-_options.RetentionDays).ToString("o"); - using var cmd = _eventLogger.Connection.CreateCommand(); - cmd.CommandText = "DELETE FROM site_events WHERE timestamp < $cutoff"; - cmd.Parameters.AddWithValue("$cutoff", cutoff); - var deleted = cmd.ExecuteNonQuery(); + var deleted = _eventLogger.WithConnection(connection => + { + using var cmd = connection.CreateCommand(); + cmd.CommandText = "DELETE FROM site_events WHERE timestamp < $cutoff"; + cmd.Parameters.AddWithValue("$cutoff", cutoff); + return cmd.ExecuteNonQuery(); + }); if (deleted > 0) { @@ -74,8 +79,8 @@ public class EventLogPurgeService : BackgroundService private void PurgeByStorageCap() { - var currentSizeBytes = GetDatabaseSizeBytes(); var capBytes = (long)_options.MaxStorageMb * 1024 * 1024; + var currentSizeBytes = GetDatabaseSizeBytes(); if (currentSizeBytes <= capBytes) return; @@ -84,37 +89,77 @@ public class EventLogPurgeService : BackgroundService "Event log size {Size:F1} MB exceeds cap {Cap} MB — purging oldest events", currentSizeBytes / (1024.0 * 1024.0), _options.MaxStorageMb); - // Delete oldest events in batches until under the cap + // Delete the oldest events in batches until the database is under the cap. + // The loop also stops if the on-disk size fails to decrease across an + // iteration (e.g. if vacuum cannot reclaim space), so a cap that can never + // be met does not silently empty the entire table. while (currentSizeBytes > capBytes) { - using var cmd = _eventLogger.Connection.CreateCommand(); - cmd.CommandText = """ - DELETE FROM site_events WHERE id IN ( - SELECT id FROM site_events ORDER BY id ASC LIMIT 1000 - ) - """; - var deleted = cmd.ExecuteNonQuery(); - if (deleted == 0) break; + var previousSizeBytes = currentSizeBytes; - // Reclaim space - using var vacuumCmd = _eventLogger.Connection.CreateCommand(); - vacuumCmd.CommandText = "PRAGMA incremental_vacuum"; - vacuumCmd.ExecuteNonQuery(); + var deleted = _eventLogger.WithConnection(connection => + { + using var cmd = connection.CreateCommand(); + cmd.CommandText = $""" + DELETE FROM site_events WHERE id IN ( + SELECT id FROM site_events ORDER BY id ASC LIMIT {CapPurgeBatchSize} + ) + """; + var rows = cmd.ExecuteNonQuery(); + + // Reclaim free pages so page_count/freelist measurement reflects the + // delete. Effective because auto_vacuum = INCREMENTAL is set at schema + // creation; harmless otherwise. + using var vacuumCmd = connection.CreateCommand(); + vacuumCmd.CommandText = "PRAGMA incremental_vacuum"; + vacuumCmd.ExecuteNonQuery(); + + return rows; + }); + + if (deleted == 0) + break; currentSizeBytes = GetDatabaseSizeBytes(); + + if (currentSizeBytes >= previousSizeBytes) + { + // Size is not shrinking despite deletes — stop rather than wipe the + // whole table. This should not happen now that logical size is + // measured, but guards against any future regression. + _logger.LogWarning( + "Event log size did not decrease after a cap-purge batch ({Size:F1} MB); " + + "stopping to avoid emptying the log", + currentSizeBytes / (1024.0 * 1024.0)); + break; + } } } + /// + /// Returns the logical size of the database in bytes — only pages that hold live + /// data, excluding free pages on the freelist. Measuring logical size (rather than + /// the raw file size from page_count) means the storage-cap loop observes + /// space being reclaimed even if free pages have not yet been returned to the OS. + /// internal long GetDatabaseSizeBytes() { - using var pageCountCmd = _eventLogger.Connection.CreateCommand(); - pageCountCmd.CommandText = "PRAGMA page_count"; - var pageCount = (long)pageCountCmd.ExecuteScalar()!; + return _eventLogger.WithConnection(connection => + { + using var pageCountCmd = connection.CreateCommand(); + pageCountCmd.CommandText = "PRAGMA page_count"; + var pageCount = (long)pageCountCmd.ExecuteScalar()!; - using var pageSizeCmd = _eventLogger.Connection.CreateCommand(); - pageSizeCmd.CommandText = "PRAGMA page_size"; - var pageSize = (long)pageSizeCmd.ExecuteScalar()!; + using var freeListCmd = connection.CreateCommand(); + freeListCmd.CommandText = "PRAGMA freelist_count"; + var freeListCount = (long)freeListCmd.ExecuteScalar()!; - return pageCount * pageSize; + using var pageSizeCmd = connection.CreateCommand(); + pageSizeCmd.CommandText = "PRAGMA page_size"; + var pageSize = (long)pageSizeCmd.ExecuteScalar()!; + + var usedPages = Math.Max(0, pageCount - freeListCount); + return usedPages * pageSize; + }); } } diff --git a/src/ScadaLink.SiteEventLogging/EventLogQueryService.cs b/src/ScadaLink.SiteEventLogging/EventLogQueryService.cs index 2b129bc..1a49820 100644 --- a/src/ScadaLink.SiteEventLogging/EventLogQueryService.cs +++ b/src/ScadaLink.SiteEventLogging/EventLogQueryService.cs @@ -33,7 +33,6 @@ public class EventLogQueryService : IEventLogQueryService { var pageSize = request.PageSize > 0 ? request.PageSize : _options.QueryPageSize; - using var cmd = _eventLogger.Connection.CreateCommand(); var whereClauses = new List(); var parameters = new List(); @@ -84,32 +83,42 @@ public class EventLogQueryService : IEventLogQueryService ? "WHERE " + string.Join(" AND ", whereClauses) : ""; - // Fetch pageSize + 1 to determine if there are more results - cmd.CommandText = $""" - SELECT id, timestamp, event_type, severity, instance_id, source, message, details - FROM site_events - {whereClause} - ORDER BY id ASC - LIMIT $limit - """; - cmd.Parameters.AddWithValue("$limit", pageSize + 1); - foreach (var p in parameters) - cmd.Parameters.Add(p); - - var entries = new List(); - using var reader = cmd.ExecuteReader(); - while (reader.Read()) + // Run the read against the shared connection under the logger's write + // lock — the connection is not thread-safe and is also used by the + // recorder and the purge service on other threads. + var entries = _eventLogger.WithConnection(connection => { - entries.Add(new EventLogEntry( - Id: reader.GetInt64(0), - Timestamp: DateTimeOffset.Parse(reader.GetString(1)), - EventType: reader.GetString(2), - Severity: reader.GetString(3), - InstanceId: reader.IsDBNull(4) ? null : reader.GetString(4), - Source: reader.GetString(5), - Message: reader.GetString(6), - Details: reader.IsDBNull(7) ? null : reader.GetString(7))); - } + using var cmd = connection.CreateCommand(); + + // Fetch pageSize + 1 to determine if there are more results + cmd.CommandText = $""" + SELECT id, timestamp, event_type, severity, instance_id, source, message, details + FROM site_events + {whereClause} + ORDER BY id ASC + LIMIT $limit + """; + cmd.Parameters.AddWithValue("$limit", pageSize + 1); + foreach (var p in parameters) + cmd.Parameters.Add(p); + + var rows = new List(); + using var reader = cmd.ExecuteReader(); + while (reader.Read()) + { + rows.Add(new EventLogEntry( + Id: reader.GetInt64(0), + Timestamp: DateTimeOffset.Parse(reader.GetString(1)), + EventType: reader.GetString(2), + Severity: reader.GetString(3), + InstanceId: reader.IsDBNull(4) ? null : reader.GetString(4), + Source: reader.GetString(5), + Message: reader.GetString(6), + Details: reader.IsDBNull(7) ? null : reader.GetString(7))); + } + + return rows; + }); var hasMore = entries.Count > pageSize; if (hasMore) diff --git a/src/ScadaLink.SiteEventLogging/SiteEventLogger.cs b/src/ScadaLink.SiteEventLogging/SiteEventLogger.cs index 34f0ea1..b51681b 100644 --- a/src/ScadaLink.SiteEventLogging/SiteEventLogger.cs +++ b/src/ScadaLink.SiteEventLogging/SiteEventLogger.cs @@ -9,6 +9,11 @@ namespace ScadaLink.SiteEventLogging; /// Only the active node generates events. Not replicated to standby. /// On failover, the new active node starts a fresh log. /// +/// +/// A single is owned here and is NOT thread-safe. +/// All access — recording, querying, purging — must be funnelled through +/// , which serialises callers on a shared lock. +/// public class SiteEventLogger : ISiteEventLogger, IDisposable { private readonly SqliteConnection _connection; @@ -31,10 +36,50 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable InitializeSchema(); } - internal SqliteConnection Connection => _connection; + /// + /// Runs against the shared connection while holding the + /// write lock, so purge / query / record callers on different threads never use + /// the non-thread-safe concurrently. + /// Returns without invoking the action if the logger has + /// been disposed. + /// + internal bool WithConnection(Action action) + { + ArgumentNullException.ThrowIfNull(action); + lock (_writeLock) + { + if (_disposed) return false; + action(_connection); + return true; + } + } + + /// + /// Runs against the shared connection while holding the + /// write lock. Throws if the logger has + /// been disposed (callers that need a result cannot proceed without the database). + /// + internal T WithConnection(Func func) + { + ArgumentNullException.ThrowIfNull(func); + lock (_writeLock) + { + ObjectDisposedException.ThrowIf(_disposed, this); + return func(_connection); + } + } private void InitializeSchema() { + // auto_vacuum must be set before any table is created for it to take effect + // on a fresh database. With INCREMENTAL mode, PRAGMA incremental_vacuum can + // later reclaim free pages so the storage-cap purge can shrink the file. + using (var pragmaCmd = _connection.CreateCommand()) + { + pragmaCmd.CommandText = "PRAGMA auto_vacuum = INCREMENTAL"; + pragmaCmd.ExecuteNonQuery(); + } + using var cmd = _connection.CreateCommand(); cmd.CommandText = """ CREATE TABLE IF NOT EXISTS site_events ( @@ -69,13 +114,11 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable var timestamp = DateTimeOffset.UtcNow.ToString("o"); - lock (_writeLock) + try { - if (_disposed) return Task.CompletedTask; - - try + WithConnection(connection => { - using var cmd = _connection.CreateCommand(); + using var cmd = connection.CreateCommand(); cmd.CommandText = """ INSERT INTO site_events (timestamp, event_type, severity, instance_id, source, message, details) VALUES ($timestamp, $event_type, $severity, $instance_id, $source, $message, $details) @@ -88,11 +131,11 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable cmd.Parameters.AddWithValue("$message", message); cmd.Parameters.AddWithValue("$details", (object?)details ?? DBNull.Value); cmd.ExecuteNonQuery(); - } - catch (Exception ex) - { - _logger.LogError(ex, "Failed to record event: {EventType} from {Source}", eventType, source); - } + }); + } + catch (Exception ex) + { + _logger.LogError(ex, "Failed to record event: {EventType} from {Source}", eventType, source); } return Task.CompletedTask; @@ -100,8 +143,11 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable public void Dispose() { - if (_disposed) return; - _disposed = true; - _connection.Dispose(); + lock (_writeLock) + { + if (_disposed) return; + _disposed = true; + _connection.Dispose(); + } } } diff --git a/tests/ScadaLink.SiteEventLogging.Tests/EventLogPurgeServiceTests.cs b/tests/ScadaLink.SiteEventLogging.Tests/EventLogPurgeServiceTests.cs index 0059802..3716c9a 100644 --- a/tests/ScadaLink.SiteEventLogging.Tests/EventLogPurgeServiceTests.cs +++ b/tests/ScadaLink.SiteEventLogging.Tests/EventLogPurgeServiceTests.cs @@ -40,20 +40,26 @@ public class EventLogPurgeServiceTests : IDisposable private void InsertEventWithTimestamp(DateTimeOffset timestamp) { - using var cmd = _eventLogger.Connection.CreateCommand(); - cmd.CommandText = """ - INSERT INTO site_events (timestamp, event_type, severity, source, message) - VALUES ($ts, 'script', 'Info', 'Test', 'Test message') - """; - cmd.Parameters.AddWithValue("$ts", timestamp.ToString("o")); - cmd.ExecuteNonQuery(); + _eventLogger.WithConnection(connection => + { + using var cmd = connection.CreateCommand(); + cmd.CommandText = """ + INSERT INTO site_events (timestamp, event_type, severity, source, message) + VALUES ($ts, 'script', 'Info', 'Test', 'Test message') + """; + cmd.Parameters.AddWithValue("$ts", timestamp.ToString("o")); + cmd.ExecuteNonQuery(); + }); } private long GetEventCount() { - using var cmd = _eventLogger.Connection.CreateCommand(); - cmd.CommandText = "SELECT COUNT(*) FROM site_events"; - return (long)cmd.ExecuteScalar()!; + return _eventLogger.WithConnection(connection => + { + using var cmd = connection.CreateCommand(); + cmd.CommandText = "SELECT COUNT(*) FROM site_events"; + return (long)cmd.ExecuteScalar()!; + }); } [Fact] @@ -116,4 +122,155 @@ public class EventLogPurgeServiceTests : IDisposable Assert.True(size > 0); } + + private void InsertBulkEvents(int count) + { + // Each event carries a sizeable details payload so the database grows + // measurably and the storage cap can be exercised against a realistic file. + var details = new string('x', 2000); + _eventLogger.WithConnection(connection => + { + for (int i = 0; i < count; i++) + { + using var cmd = connection.CreateCommand(); + cmd.CommandText = """ + INSERT INTO site_events (timestamp, event_type, severity, source, message, details) + VALUES ($ts, 'script', 'Info', 'Test', 'Bulk event', $details) + """; + cmd.Parameters.AddWithValue("$ts", DateTimeOffset.UtcNow.ToString("o")); + cmd.Parameters.AddWithValue("$details", details); + cmd.ExecuteNonQuery(); + } + }); + } + + private long MinEventId() + { + return _eventLogger.WithConnection(connection => + { + using var cmd = connection.CreateCommand(); + cmd.CommandText = "SELECT MIN(id) FROM site_events"; + var result = cmd.ExecuteScalar(); + return result is long l ? l : 0; + }); + } + + [Fact] + public void PurgeByStorageCap_StopsWhenUnderCap_DoesNotEmptyTable() + { + // Regression test for SiteEventLogging-001 / -002: + // a realistic non-zero cap must trim the oldest events to the budget, + // not delete the entire table. + InsertBulkEvents(3000); + + var purge = CreatePurgeService(); + var totalSize = purge.GetDatabaseSizeBytes(); + + // Cap at roughly half the current database size — purge must keep some rows. + var capBytes = totalSize / 2; + var capOptions = new SiteEventLogOptions + { + DatabasePath = _dbPath, + RetentionDays = 30, + MaxStorageMb = (int)Math.Max(1, capBytes / (1024 * 1024)) + }; + + var cappedPurge = CreatePurgeService(capOptions); + cappedPurge.RunPurge(); + + var remaining = GetEventCount(); + Assert.True(remaining > 0, "Storage-cap purge must not delete the entire table."); + Assert.True(remaining < 3000, "Storage-cap purge must remove some events when over cap."); + + // The database must actually be back under the cap after purge. + var finalSize = cappedPurge.GetDatabaseSizeBytes(); + var finalCapBytes = (long)capOptions.MaxStorageMb * 1024 * 1024; + Assert.True(finalSize <= finalCapBytes, + $"Database size {finalSize} must be at or below cap {finalCapBytes} after purge."); + } + + [Fact] + public void PurgeByStorageCap_RemovesOldestEventsFirst() + { + // Regression test for SiteEventLogging-002: only the oldest events + // (lowest ids) should be removed when trimming to the cap. + InsertBulkEvents(3000); + + var purge = CreatePurgeService(); + var totalSize = purge.GetDatabaseSizeBytes(); + + var capOptions = new SiteEventLogOptions + { + DatabasePath = _dbPath, + RetentionDays = 30, + MaxStorageMb = (int)Math.Max(1, (totalSize / 2) / (1024 * 1024)) + }; + + var minIdBefore = MinEventId(); + var cappedPurge = CreatePurgeService(capOptions); + cappedPurge.RunPurge(); + var minIdAfter = MinEventId(); + + // The surviving rows must be the newest ones — minimum id has advanced. + Assert.True(minIdAfter > minIdBefore, + "Oldest events (lowest ids) must be purged first."); + + // The newest event (highest id) must still be present. + var newestPresent = _eventLogger.WithConnection(connection => + { + using var cmd = connection.CreateCommand(); + cmd.CommandText = "SELECT COUNT(*) FROM site_events WHERE id = 3000"; + return (long)cmd.ExecuteScalar()!; + }); + Assert.Equal(1L, newestPresent); + } + + [Fact] + public async Task PurgeByStorageCap_ConcurrentWritesDoNotCorruptConnection() + { + // Regression test for SiteEventLogging-003: purge running on a background + // thread while events are recorded on other threads must not throw + // "DataReader already open" / "connection busy" from a shared connection. + InsertBulkEvents(2000); + + var purge = CreatePurgeService(); + var totalSize = purge.GetDatabaseSizeBytes(); + var capOptions = new SiteEventLogOptions + { + DatabasePath = _dbPath, + RetentionDays = 30, + MaxStorageMb = (int)Math.Max(1, (totalSize / 2) / (1024 * 1024)) + }; + + var exceptions = new System.Collections.Concurrent.ConcurrentBag(); + var stop = false; + + var purgeTask = Task.Run(() => + { + try + { + var p = CreatePurgeService(capOptions); + for (int i = 0; i < 20; i++) p.RunPurge(); + } + catch (Exception ex) { exceptions.Add(ex); } + }); + + var writeTasks = Enumerable.Range(0, 4).Select(_ => Task.Run(async () => + { + try + { + while (!stop) + { + await _eventLogger.LogEventAsync("script", "Info", null, "Concurrent", "Concurrent write"); + } + } + catch (Exception ex) { exceptions.Add(ex); } + })).ToArray(); + + await purgeTask; + stop = true; + await Task.WhenAll(writeTasks); + + Assert.Empty(exceptions); + } } diff --git a/tests/ScadaLink.SiteEventLogging.Tests/EventLogQueryServiceTests.cs b/tests/ScadaLink.SiteEventLogging.Tests/EventLogQueryServiceTests.cs index 2f6b55f..196b585 100644 --- a/tests/ScadaLink.SiteEventLogging.Tests/EventLogQueryServiceTests.cs +++ b/tests/ScadaLink.SiteEventLogging.Tests/EventLogQueryServiceTests.cs @@ -256,17 +256,20 @@ public class EventLogQueryServiceTests : IDisposable private void InsertEventAt(DateTimeOffset timestamp, string eventType, string severity, string? instanceId, string source, string message) { - using var cmd = _eventLogger.Connection.CreateCommand(); - cmd.CommandText = """ - INSERT INTO site_events (timestamp, event_type, severity, instance_id, source, message) - VALUES ($ts, $et, $sev, $iid, $src, $msg) - """; - cmd.Parameters.AddWithValue("$ts", timestamp.ToString("o")); - cmd.Parameters.AddWithValue("$et", eventType); - cmd.Parameters.AddWithValue("$sev", severity); - cmd.Parameters.AddWithValue("$iid", (object?)instanceId ?? DBNull.Value); - cmd.Parameters.AddWithValue("$src", source); - cmd.Parameters.AddWithValue("$msg", message); - cmd.ExecuteNonQuery(); + _eventLogger.WithConnection(connection => + { + using var cmd = connection.CreateCommand(); + cmd.CommandText = """ + INSERT INTO site_events (timestamp, event_type, severity, instance_id, source, message) + VALUES ($ts, $et, $sev, $iid, $src, $msg) + """; + cmd.Parameters.AddWithValue("$ts", timestamp.ToString("o")); + cmd.Parameters.AddWithValue("$et", eventType); + cmd.Parameters.AddWithValue("$sev", severity); + cmd.Parameters.AddWithValue("$iid", (object?)instanceId ?? DBNull.Value); + cmd.Parameters.AddWithValue("$src", source); + cmd.Parameters.AddWithValue("$msg", message); + cmd.ExecuteNonQuery(); + }); } }