From 22fa7c5b55ead52cfae06ee4f32d4ae536ae9812 Mon Sep 17 00:00:00 2001 From: HeavenVR Date: Wed, 1 Jul 2026 12:45:03 +0200 Subject: [PATCH 1/3] test(cron): cover the control-log retention trim end-to-end against real Postgres Extract ClearOldShockerControlLogs' inline CTE DELETE into ShockerControlLogQueries.DeleteControlLogsBeyondPerUserLimitAsync (Common) as a single source of truth, parameterized by the per-user cap so tests can drive it with a small limit. The job now calls it with HardLimits.MaxShockerControlLogsPerUser. Add a Cron integration test (Cron.IntegrationTests, alongside the delivery tests) that seeds a user -> device -> shocker -> logs graph and runs the actual statement against a Testcontainers Postgres: it exercises the real table/column names, the shocker -> device -> owner join, the per-user window function, and newest-first ordering, asserting the newest N survive and the rest are deleted. A schema rename now fails the test rather than only the Cron host at runtime. Mirrors the outbox claim-query extraction (#328). No behavior change to the job. --- .../OpenShockDb/ShockerControlLogQueries.cs | 40 +++++++ .../Tests/ControlLogRetentionTests.cs | 103 ++++++++++++++++++ Cron/Jobs/ClearOldShockerControlLogs.cs | 20 +--- 3 files changed, 146 insertions(+), 17 deletions(-) create mode 100644 Common/OpenShockDb/ShockerControlLogQueries.cs create mode 100644 Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs diff --git a/Common/OpenShockDb/ShockerControlLogQueries.cs b/Common/OpenShockDb/ShockerControlLogQueries.cs new file mode 100644 index 00000000..04080dfa --- /dev/null +++ b/Common/OpenShockDb/ShockerControlLogQueries.cs @@ -0,0 +1,40 @@ +using Microsoft.EntityFrameworkCore; + +namespace OpenShock.Common.OpenShockDb; + +/// +/// Reusable commands over the shocker_control_logs table. Kept here (rather than inline in the +/// Cron cleanup job) so the raw SQL is a single source of truth that integration tests can execute +/// directly - a table or column rename then breaks the test rather than surfacing only in production. +/// +public static class ShockerControlLogQueries +{ + /// + /// Retention trim: per owning user, keeps the newest control logs (by + /// created_at) and deletes the rest. Ownership is resolved through + /// shocker -> device -> owner, so the cap is per user, not per shocker or device. Runs as a + /// single set-based statement (a window function ranks each user's logs newest-first; anything past the + /// cap is deleted). Returns the number of rows deleted. + /// + public static Task DeleteControlLogsBeyondPerUserLimitAsync(this OpenShockContext db, int maxPerUser, + CancellationToken cancellationToken = default) + { + if (maxPerUser < 0) throw new ArgumentOutOfRangeException(nameof(maxPerUser)); + + return db.Database.ExecuteSqlAsync( + $""" + WITH ranked_logs AS ( + SELECT + l.id, + ROW_NUMBER() OVER (PARTITION BY d.owner_id ORDER BY l.created_at DESC) AS rn + FROM shocker_control_logs l + JOIN shockers s ON s.id = l.shocker_id + JOIN devices d ON d.id = s.device_id + ) + DELETE FROM shocker_control_logs l + USING ranked_logs rl + WHERE l.id = rl.id + AND rl.rn > {maxPerUser} + """, cancellationToken); + } +} diff --git a/Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs b/Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs new file mode 100644 index 00000000..3e92599d --- /dev/null +++ b/Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs @@ -0,0 +1,103 @@ +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using OpenShock.Common.Models; +using OpenShock.Common.OpenShockDb; +using OpenShock.Common.Utils; + +namespace OpenShock.Cron.IntegrationTests.Tests; + +/// +/// End-to-end coverage for the Cron retention trim against real Postgres. Runs the cleanup job's *actual* +/// statement (, the single +/// source of truth the job uses) over a seeded user -> device -> shocker -> logs graph, so the real table +/// and column names, the shocker -> device -> owner join, the per-user window function, and the newest-first +/// ordering are all exercised. A schema rename breaks this test rather than only the Cron host at runtime. +/// +public sealed class ControlLogRetentionTests +{ + [ClassDataSource(Shared = SharedType.PerTestSession)] + public required CronApplicationFactory Factory { get; init; } + + [Test] + public async Task DeleteControlLogsBeyondPerUserLimit_KeepsNewestPerUser_AndDeletesTheRest() + { + const int keep = 10; + const int seed = 15; // 5 over the limit + + var userId = Guid.CreateVersion7(); + var deviceId = Guid.CreateVersion7(); + var shockerId = Guid.CreateVersion7(); + + // Seed `seed` logs with strictly increasing timestamps so newest-first ordering is unambiguous. + // Track the ids in age order; the newest `keep` should survive, the oldest `seed - keep` should go. + var idsOldestToNewest = new List(seed); + var baseTime = DateTime.UtcNow - TimeSpan.FromDays(1); + + await using (var db = await Factory.DbContextFactory.CreateDbContextAsync()) + { + db.Users.Add(new User + { + Id = userId, + Name = $"logtrim{userId:N}"[..16], + Email = $"logtrim-{userId:N}@test.org", + SecurityStamp = Guid.CreateVersion7(), + CreatedAt = DateTime.UtcNow, + ActivatedAt = DateTime.UtcNow + }); + db.Devices.Add(new Device + { + Id = deviceId, OwnerId = userId, Name = "LogTrimHub", + Token = CryptoUtils.RandomAlphaNumericString(256), CreatedAt = DateTime.UtcNow + }); + db.Shockers.Add(new Shocker + { + Id = shockerId, Name = "LogTrimShocker", RfId = 1234, + DeviceId = deviceId, Model = ShockerModelType.CaiXianlin + }); + + for (var i = 0; i < seed; i++) + { + var logId = Guid.CreateVersion7(); + idsOldestToNewest.Add(logId); + db.ShockerControlLogs.Add(new ShockerControlLog + { + Id = logId, + ShockerId = shockerId, + ControlledByUserId = userId, + Intensity = 50, + Duration = 1000, + Type = ControlType.Shock, + CustomName = null, + CreatedAt = baseTime + TimeSpan.FromMinutes(i) + }); + } + + await db.SaveChangesAsync(); + } + + // Resolve the pooled OpenShockContext exactly as the Cron cleanup job does (DI-injected) so this + // covers the same registration path that runs in production. + using var scope = Factory.Services.CreateScope(); + var db2 = scope.ServiceProvider.GetRequiredService(); + + var deleted = await db2.DeleteControlLogsBeyondPerUserLimitAsync(keep); + + // The trim is global, but no other integration test writes shocker control logs, so this user's rows + // are the only ones in the table: exactly the oldest `seed - keep` are deleted and the newest `keep` + // remain. The count therefore equals this user's deletion. + await Assert.That(deleted).IsEqualTo(seed - keep); + + var survivingIds = await db2.ShockerControlLogs + .AsNoTracking() + .Where(l => l.ShockerId == shockerId) + .Select(l => l.Id) + .ToListAsync(); + + var expectedSurviving = idsOldestToNewest.Skip(seed - keep).ToHashSet(); + var expectedDeleted = idsOldestToNewest.Take(seed - keep).ToHashSet(); + + await Assert.That(survivingIds.Count).IsEqualTo(keep); + await Assert.That(survivingIds.ToHashSet().SetEquals(expectedSurviving)).IsTrue(); + await Assert.That(survivingIds.Any(id => expectedDeleted.Contains(id))).IsFalse(); + } +} diff --git a/Cron/Jobs/ClearOldShockerControlLogs.cs b/Cron/Jobs/ClearOldShockerControlLogs.cs index 27733bde..5f2cf198 100644 --- a/Cron/Jobs/ClearOldShockerControlLogs.cs +++ b/Cron/Jobs/ClearOldShockerControlLogs.cs @@ -1,5 +1,4 @@ -using Microsoft.EntityFrameworkCore; -using OpenShock.Common.Constants; +using OpenShock.Common.Constants; using OpenShock.Common.OpenShockDb; using OpenShock.Cron.Attributes; @@ -27,21 +26,8 @@ public ClearOldShockerControlLogs(OpenShockContext db, ILogger Execute() { - var deletedUserLimits = await _db.Database.ExecuteSqlAsync( - $""" - WITH ranked_logs AS ( - SELECT - l.id, - ROW_NUMBER() OVER (PARTITION BY d.owner_id ORDER BY l.created_at DESC) AS rn - FROM shocker_control_logs l - JOIN shockers s ON s.id = l.shocker_id - JOIN devices d ON d.id = s.device_id - ) - DELETE FROM shocker_control_logs l - USING ranked_logs rl - WHERE l.id = rl.id - AND rl.rn > {HardLimits.MaxShockerControlLogsPerUser} - """); + var deletedUserLimits = + await _db.DeleteControlLogsBeyondPerUserLimitAsync(HardLimits.MaxShockerControlLogsPerUser); _logger.LogInformation("Deleted {deletedUserLimits} shocker control logs exceeding the per-user limit", deletedUserLimits); From 046e239cfda5741625fc12416bae4f76edb67944 Mon Sep 17 00:00:00 2001 From: HeavenVR Date: Wed, 1 Jul 2026 13:08:52 +0200 Subject: [PATCH 2/3] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs b/Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs index 3e92599d..7b829fa1 100644 --- a/Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs +++ b/Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs @@ -77,7 +77,7 @@ public async Task DeleteControlLogsBeyondPerUserLimit_KeepsNewestPerUser_AndDele // Resolve the pooled OpenShockContext exactly as the Cron cleanup job does (DI-injected) so this // covers the same registration path that runs in production. - using var scope = Factory.Services.CreateScope(); + await using var scope = Factory.Services.CreateAsyncScope(); var db2 = scope.ServiceProvider.GetRequiredService(); var deleted = await db2.DeleteControlLogsBeyondPerUserLimitAsync(keep); From fd78421f932ec37b4d8af07fd7a7a71b9465b568 Mon Sep 17 00:00:00 2001 From: HeavenVR Date: Wed, 1 Jul 2026 13:14:32 +0200 Subject: [PATCH 3/3] test(cron): make control-log trim test prove per-owner partitioning Per Copilot review: the single-owner setup (where the log's controller equalled the device owner) couldn't tell PARTITION BY d.owner_id apart from a buggy partition on controlled_by_user_id or a dropped PARTITION BY. Strengthen it: seed a second owner under the limit whose (older) logs must all survive, and attribute the over-limit owner's logs to two distinct controller users (neither over the limit). Now the assertions fail if the query ranks globally (would delete 10 and wipe the under-limit owner's logs) or partitions by controller (would delete 0) - only a correct per-owner trim deletes exactly the owner's oldest 5. Keeps the await-using async scope from the prior autofix commit. --- .../Tests/ControlLogRetentionTests.cs | 146 +++++++++++------- 1 file changed, 90 insertions(+), 56 deletions(-) diff --git a/Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs b/Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs index 7b829fa1..e4f436d9 100644 --- a/Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs +++ b/Cron.IntegrationTests/Tests/ControlLogRetentionTests.cs @@ -9,9 +9,9 @@ namespace OpenShock.Cron.IntegrationTests.Tests; /// /// End-to-end coverage for the Cron retention trim against real Postgres. Runs the cleanup job's *actual* /// statement (, the single -/// source of truth the job uses) over a seeded user -> device -> shocker -> logs graph, so the real table -/// and column names, the shocker -> device -> owner join, the per-user window function, and the newest-first -/// ordering are all exercised. A schema rename breaks this test rather than only the Cron host at runtime. +/// source of truth the job uses) over a seeded graph, so the real table/column names, the +/// shocker -> device -> owner join, the per-user window function, and newest-first ordering are exercised. +/// A schema rename breaks this test rather than only the Cron host at runtime. /// public sealed class ControlLogRetentionTests { @@ -19,57 +19,55 @@ public sealed class ControlLogRetentionTests public required CronApplicationFactory Factory { get; init; } [Test] - public async Task DeleteControlLogsBeyondPerUserLimit_KeepsNewestPerUser_AndDeletesTheRest() + public async Task DeleteControlLogsBeyondPerUserLimit_TrimsPerOwner_KeepsNewest_AndSparesOwnersUnderTheLimit() { const int keep = 10; - const int seed = 15; // 5 over the limit - var userId = Guid.CreateVersion7(); - var deviceId = Guid.CreateVersion7(); - var shockerId = Guid.CreateVersion7(); + // Owner A is over the limit; owner B is under it. A's logs are attributed to two *different* + // controller users (each under the limit on its own) and B's logs to B itself - so the trim is only + // correct if it partitions by the shocker's OWNER (d.owner_id). If it instead partitioned by the + // controller (l.controlled_by_user_id) it would delete nothing (no controller exceeds the limit); if + // it dropped PARTITION BY entirely it would rank globally and wrongly delete B's (older) logs too. + var ownerA = Guid.CreateVersion7(); + var ownerB = Guid.CreateVersion7(); + var controllerC = Guid.CreateVersion7(); + var controllerD = Guid.CreateVersion7(); + + var shockerA = Guid.CreateVersion7(); + var shockerB = Guid.CreateVersion7(); - // Seed `seed` logs with strictly increasing timestamps so newest-first ordering is unambiguous. - // Track the ids in age order; the newest `keep` should survive, the oldest `seed - keep` should go. - var idsOldestToNewest = new List(seed); var baseTime = DateTime.UtcNow - TimeSpan.FromDays(1); + // A: 15 logs (5 over). Newest 10 survive, oldest 5 go. Split across controllers C (first 8) and D. + var aOldestToNewest = new List(15); + // B: 5 logs, all older than A's - a global (unpartitioned) trim would wrongly delete them. + var bIds = new List(5); + await using (var db = await Factory.DbContextFactory.CreateDbContextAsync()) { - db.Users.Add(new User - { - Id = userId, - Name = $"logtrim{userId:N}"[..16], - Email = $"logtrim-{userId:N}@test.org", - SecurityStamp = Guid.CreateVersion7(), - CreatedAt = DateTime.UtcNow, - ActivatedAt = DateTime.UtcNow - }); - db.Devices.Add(new Device - { - Id = deviceId, OwnerId = userId, Name = "LogTrimHub", - Token = CryptoUtils.RandomAlphaNumericString(256), CreatedAt = DateTime.UtcNow - }); - db.Shockers.Add(new Shocker + db.Users.Add(NewUser(ownerA, "own-a")); + db.Users.Add(NewUser(ownerB, "own-b")); + db.Users.Add(NewUser(controllerC, "ctl-c")); + db.Users.Add(NewUser(controllerD, "ctl-d")); + + AddShocker(db, shockerB, ownerB, rfId: 2000); + AddShocker(db, shockerA, ownerA, rfId: 1000); + + // B's logs sit at minutes 0..4 (oldest overall) so a global newest-10 trim would drop them. + for (var i = 0; i < 5; i++) { - Id = shockerId, Name = "LogTrimShocker", RfId = 1234, - DeviceId = deviceId, Model = ShockerModelType.CaiXianlin - }); + var logId = Guid.CreateVersion7(); + bIds.Add(logId); + db.ShockerControlLogs.Add(NewLog(logId, shockerB, ownerB, baseTime + TimeSpan.FromMinutes(i))); + } - for (var i = 0; i < seed; i++) + // A's logs sit at minutes 100..114 (newest overall), the first 8 by C and the last 7 by D. + for (var i = 0; i < 15; i++) { var logId = Guid.CreateVersion7(); - idsOldestToNewest.Add(logId); - db.ShockerControlLogs.Add(new ShockerControlLog - { - Id = logId, - ShockerId = shockerId, - ControlledByUserId = userId, - Intensity = 50, - Duration = 1000, - Type = ControlType.Shock, - CustomName = null, - CreatedAt = baseTime + TimeSpan.FromMinutes(i) - }); + aOldestToNewest.Add(logId); + var controller = i < 8 ? controllerC : controllerD; + db.ShockerControlLogs.Add(NewLog(logId, shockerA, controller, baseTime + TimeSpan.FromMinutes(100 + i))); } await db.SaveChangesAsync(); @@ -82,22 +80,58 @@ public async Task DeleteControlLogsBeyondPerUserLimit_KeepsNewestPerUser_AndDele var deleted = await db2.DeleteControlLogsBeyondPerUserLimitAsync(keep); - // The trim is global, but no other integration test writes shocker control logs, so this user's rows - // are the only ones in the table: exactly the oldest `seed - keep` are deleted and the newest `keep` - // remain. The count therefore equals this user's deletion. - await Assert.That(deleted).IsEqualTo(seed - keep); + // Only owner A is over the limit, so exactly its oldest 5 are removed. No other integration test + // writes control logs, so the global count equals A's deletion: 5. (Dropping PARTITION BY would + // delete 10; partitioning by controller instead of owner would delete 0.) + await Assert.That(deleted).IsEqualTo(5); - var survivingIds = await db2.ShockerControlLogs - .AsNoTracking() - .Where(l => l.ShockerId == shockerId) - .Select(l => l.Id) - .ToListAsync(); + var survivingA = (await db2.ShockerControlLogs.AsNoTracking() + .Where(l => l.ShockerId == shockerA).Select(l => l.Id).ToListAsync()).ToHashSet(); + var survivingB = await db2.ShockerControlLogs.AsNoTracking() + .Where(l => l.ShockerId == shockerB).Select(l => l.Id).ToListAsync(); - var expectedSurviving = idsOldestToNewest.Skip(seed - keep).ToHashSet(); - var expectedDeleted = idsOldestToNewest.Take(seed - keep).ToHashSet(); + // A keeps its newest 10; its oldest 5 are gone. + await Assert.That(survivingA.SetEquals(aOldestToNewest.Skip(5).ToHashSet())).IsTrue(); + await Assert.That(aOldestToNewest.Take(5).Any(survivingA.Contains)).IsFalse(); - await Assert.That(survivingIds.Count).IsEqualTo(keep); - await Assert.That(survivingIds.ToHashSet().SetEquals(expectedSurviving)).IsTrue(); - await Assert.That(survivingIds.Any(id => expectedDeleted.Contains(id))).IsFalse(); + // B is under the limit, so every one of its (older) logs survives - a global trim would have deleted them. + await Assert.That(survivingB.Count).IsEqualTo(5); } + + private static User NewUser(Guid id, string prefix) => new() + { + Id = id, + Name = $"{prefix}{id:N}"[..16], + Email = $"{prefix}-{id:N}@test.org", + SecurityStamp = Guid.CreateVersion7(), + CreatedAt = DateTime.UtcNow, + ActivatedAt = DateTime.UtcNow + }; + + private static void AddShocker(OpenShockContext db, Guid shockerId, Guid ownerId, ushort rfId) + { + var deviceId = Guid.CreateVersion7(); + db.Devices.Add(new Device + { + Id = deviceId, OwnerId = ownerId, Name = "LogTrimHub", + Token = CryptoUtils.RandomAlphaNumericString(256), CreatedAt = DateTime.UtcNow + }); + db.Shockers.Add(new Shocker + { + Id = shockerId, Name = "LogTrimShocker", RfId = rfId, + DeviceId = deviceId, Model = ShockerModelType.CaiXianlin + }); + } + + private static ShockerControlLog NewLog(Guid id, Guid shockerId, Guid controllerId, DateTime createdAt) => new() + { + Id = id, + ShockerId = shockerId, + ControlledByUserId = controllerId, + Intensity = 50, + Duration = 1000, + Type = ControlType.Shock, + CustomName = null, + CreatedAt = createdAt + }; }