Skip to content

Commit 154223e

Browse files
niemyjskiCopilot
niemyjski
and
Copilot
committed
fix: optimize cleanup jobs, eliminate raw ES client usage, add test coverage
- Replace lossy partitioned terms aggregations with composite aggregations for correct pagination across large datasets - Remove ALL direct IElasticClient usage from CleanupOrphanedDataJob; encapsulate behind repository methods (IEventRepository, IStackRepository) - Fix Painless script injection vulnerability in ReassignStack (string interpolation → ScriptPatch with params) - Increase batch size 5→100, lock timeout 15min→2hr, add cancellation checks - Add 11 new tests: 7 for CleanupOrphanedDataJob, 4 for CleanupDataJob - New repository methods: RemoveAllByProjectIdsAsync, RemoveAllByOrganizationIdsAsync, ReassignStackAsync, GetDistinctStackIdsAsync, GetDistinctProjectIdsAsync, GetDistinctOrganizationIdsAsync, GetDuplicateSignaturesAsync Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent f3f6356 commit 154223e

8 files changed

Lines changed: 655 additions & 251 deletions

File tree

‎src/Exceptionless.Core/Jobs/CleanupDataJob.cs‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ ILoggerFactory loggerFactory
6464

6565
protected override Task<ILock?> GetLockAsync(CancellationToken cancellationToken = default)
6666
{
67-
return _lockProvider.TryAcquireAsync(nameof(CleanupDataJob), TimeSpan.FromMinutes(15), cancellationToken);
67+
return _lockProvider.TryAcquireAsync(nameof(CleanupDataJob), TimeSpan.FromHours(2), cancellationToken);
6868
}
6969

7070
protected override async Task<JobResult> RunInternalAsync(JobContext context)
@@ -101,13 +101,16 @@ private async Task MarkTokensSuspended(JobContext context)
101101

102102
private async Task CleanupSoftDeletedOrganizationsAsync(JobContext context)
103103
{
104-
var organizationResults = await _organizationRepository.GetAllAsync(o => o.SoftDeleteMode(SoftDeleteQueryMode.DeletedOnly).SearchAfterPaging().PageLimit(5));
104+
var organizationResults = await _organizationRepository.GetAllAsync(o => o.SoftDeleteMode(SoftDeleteQueryMode.DeletedOnly).SearchAfterPaging().PageLimit(100));
105105
_logger.CleanupOrganizationSoftDeletes(organizationResults.Total);
106106

107107
while (organizationResults.Documents.Count > 0 && !context.CancellationToken.IsCancellationRequested)
108108
{
109109
foreach (var organization in organizationResults.Documents)
110110
{
111+
if (context.CancellationToken.IsCancellationRequested)
112+
break;
113+
111114
using var _ = _logger.BeginScope(new ExceptionlessState().Organization(organization.Id));
112115
try
113116
{
@@ -129,13 +132,16 @@ private async Task CleanupSoftDeletedOrganizationsAsync(JobContext context)
129132

130133
private async Task CleanupSoftDeletedProjectsAsync(JobContext context)
131134
{
132-
var projectResults = await _projectRepository.GetAllAsync(o => o.SoftDeleteMode(SoftDeleteQueryMode.DeletedOnly).SearchAfterPaging().PageLimit(5));
135+
var projectResults = await _projectRepository.GetAllAsync(o => o.SoftDeleteMode(SoftDeleteQueryMode.DeletedOnly).SearchAfterPaging().PageLimit(100));
133136
_logger.CleanupProjectSoftDeletes(projectResults.Total);
134137

135138
while (projectResults.Documents.Count > 0 && !context.CancellationToken.IsCancellationRequested)
136139
{
137140
foreach (var project in projectResults.Documents)
138141
{
142+
if (context.CancellationToken.IsCancellationRequested)
143+
break;
144+
139145
using var _ = _logger.BeginScope(new ExceptionlessState().Organization(project.OrganizationId).Project(project.Id));
140146
try
141147
{
@@ -233,6 +239,9 @@ private async Task EnforceRetentionAsync(JobContext context)
233239
{
234240
foreach (var organization in results.Documents)
235241
{
242+
if (context.CancellationToken.IsCancellationRequested)
243+
break;
244+
236245
using var _ = _logger.BeginScope(new ExceptionlessState().Organization(organization.Id));
237246

238247
int retentionDays = _billingManager.GetBillingPlanByUpsellingRetentionPeriod(organization.RetentionDays)?.RetentionDays ?? _appOptions.MaximumRetentionDays;

‎src/Exceptionless.Core/Jobs/CleanupOrphanedDataJob.cs‎

Lines changed: 153 additions & 247 deletions
Large diffs are not rendered by default.

‎src/Exceptionless.Core/Repositories/EventRepository.cs‎

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
using Exceptionless.Core.Models;
1+
using System.Linq.Expressions;
2+
using Exceptionless.Core.Models;
23
using Exceptionless.Core.Repositories.Configuration;
34
using Exceptionless.Core.Repositories.Queries;
45
using Exceptionless.Core.Validation;
@@ -11,11 +12,13 @@ namespace Exceptionless.Core.Repositories;
1112

1213
public class EventRepository : RepositoryOwnedByOrganizationAndProject<PersistentEvent>, IEventRepository
1314
{
15+
private readonly ExceptionlessElasticConfiguration _configuration;
1416
private readonly TimeProvider _timeProvider;
1517

1618
public EventRepository(ExceptionlessElasticConfiguration configuration, AppOptions options, MiniValidationValidator validator)
1719
: base(configuration.Events, validator, options)
1820
{
21+
_configuration = configuration;
1922
_timeProvider = configuration.TimeProvider;
2023

2124
DisableCache(); // NOTE: If cache is ever enabled, then fast paths for patching/deleting with scripts will be super slow!
@@ -194,4 +197,81 @@ public Task<long> RemoveAllByStackIdsAsync(string[] stackIds)
194197

195198
return RemoveAllAsync(q => q.Stack(stackIds));
196199
}
200+
201+
public Task<long> RemoveAllByProjectIdsAsync(string[] projectIds)
202+
{
203+
ArgumentNullException.ThrowIfNull(projectIds);
204+
if (projectIds.Length == 0)
205+
throw new ArgumentOutOfRangeException(nameof(projectIds));
206+
207+
return RemoveAllAsync(q => q.Project(projectIds));
208+
}
209+
210+
public Task<long> RemoveAllByOrganizationIdsAsync(string[] organizationIds)
211+
{
212+
ArgumentNullException.ThrowIfNull(organizationIds);
213+
if (organizationIds.Length == 0)
214+
throw new ArgumentOutOfRangeException(nameof(organizationIds));
215+
216+
return RemoveAllAsync(q => q.Organization(organizationIds));
217+
}
218+
219+
public Task<long> ReassignStackAsync(IEnumerable<string> sourceStackIds, string targetStackId)
220+
{
221+
ArgumentNullException.ThrowIfNull(sourceStackIds);
222+
ArgumentException.ThrowIfNullOrEmpty(targetStackId);
223+
224+
return PatchAllAsync(
225+
q => q.Stack(sourceStackIds),
226+
new ScriptPatch("ctx._source.stack_id = params.targetStackId")
227+
{
228+
Params = new Dictionary<string, object> { ["targetStackId"] = targetStackId }
229+
});
230+
}
231+
232+
public Task<IReadOnlyCollection<string>> GetDistinctStackIdsAsync(int batchSize, CompositeKeyResult? afterKey = null)
233+
{
234+
return GetDistinctFieldValuesAsync("stack_id", e => e.StackId, batchSize, afterKey);
235+
}
236+
237+
public Task<IReadOnlyCollection<string>> GetDistinctProjectIdsAsync(int batchSize, CompositeKeyResult? afterKey = null)
238+
{
239+
return GetDistinctFieldValuesAsync("project_id", e => e.ProjectId, batchSize, afterKey);
240+
}
241+
242+
public Task<IReadOnlyCollection<string>> GetDistinctOrganizationIdsAsync(int batchSize, CompositeKeyResult? afterKey = null)
243+
{
244+
return GetDistinctFieldValuesAsync("organization_id", e => e.OrganizationId, batchSize, afterKey);
245+
}
246+
247+
private async Task<IReadOnlyCollection<string>> GetDistinctFieldValuesAsync(
248+
string fieldName, Expression<Func<PersistentEvent, object>> fieldExpression, int batchSize, CompositeKeyResult? afterKey)
249+
{
250+
var search = await _configuration.Client.SearchAsync<PersistentEvent>(s =>
251+
{
252+
s.Size(0).Aggregations(a => a
253+
.Composite($"composite_{fieldName}", c =>
254+
{
255+
var composite = c.Size(batchSize)
256+
.Sources(src => src.Terms(fieldName, t => t.Field(fieldExpression)));
257+
if (afterKey?.AfterKey.Count > 0)
258+
composite.After(new CompositeKey(afterKey.AfterKey));
259+
return composite;
260+
}));
261+
return s;
262+
});
263+
264+
var composite = search.Aggregations.Composite($"composite_{fieldName}");
265+
if (composite?.Buckets == null || composite.Buckets.Count == 0)
266+
return Array.Empty<string>();
267+
268+
if (composite.AfterKey != null && afterKey != null)
269+
{
270+
afterKey.AfterKey.Clear();
271+
foreach (var kvp in composite.AfterKey)
272+
afterKey.AfterKey[kvp.Key] = kvp.Value;
273+
}
274+
275+
return composite.Buckets.Select(b => b.Key[fieldName].ToString()!).ToArray();
276+
}
197277
}

‎src/Exceptionless.Core/Repositories/Interfaces/IEventRepository.cs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,17 @@ public interface IEventRepository : IRepositoryOwnedByOrganizationAndProject<Per
1313
Task<bool> UpdateSessionStartLastActivityAsync(string id, DateTime lastActivityUtc, bool isSessionEnd = false, bool hasError = false, bool sendNotifications = true);
1414
Task<long> RemoveAllAsync(string organizationId, string? clientIpAddress, DateTime? utcStart, DateTime? utcEnd, CommandOptionsDescriptor<PersistentEvent>? options = null);
1515
Task<long> RemoveAllByStackIdsAsync(string[] stackIds);
16+
Task<long> RemoveAllByProjectIdsAsync(string[] projectIds);
17+
Task<long> RemoveAllByOrganizationIdsAsync(string[] organizationIds);
18+
Task<long> ReassignStackAsync(IEnumerable<string> sourceStackIds, string targetStackId);
19+
Task<IReadOnlyCollection<string>> GetDistinctStackIdsAsync(int batchSize, CompositeKeyResult? afterKey = null);
20+
Task<IReadOnlyCollection<string>> GetDistinctProjectIdsAsync(int batchSize, CompositeKeyResult? afterKey = null);
21+
Task<IReadOnlyCollection<string>> GetDistinctOrganizationIdsAsync(int batchSize, CompositeKeyResult? afterKey = null);
22+
}
23+
24+
public class CompositeKeyResult
25+
{
26+
public Dictionary<string, object> AfterKey { get; set; } = new();
1627
}
1728

1829
public static class EventRepositoryExtensions

‎src/Exceptionless.Core/Repositories/Interfaces/IStackRepository.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,4 +15,5 @@ public interface IStackRepository : IRepositoryOwnedByOrganizationAndProject<Sta
1515
Task<FindResults<Stack>> GetStacksForCleanupAsync(string organizationId, DateTime cutoff);
1616
Task<FindResults<Stack>> GetSoftDeleted();
1717
Task<long> SoftDeleteByProjectIdAsync(string organizationId, string projectId);
18+
Task<IReadOnlyCollection<string>> GetDuplicateSignaturesAsync(int maxResults = 10000);
1819
}

‎src/Exceptionless.Core/Repositories/StackRepository.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,18 @@ public Task<long> SoftDeleteByProjectIdAsync(string organizationId, string proje
188188
);
189189
}
190190

191+
public async Task<IReadOnlyCollection<string>> GetDuplicateSignaturesAsync(int maxResults = 10000)
192+
{
193+
var result = await CountAsync(q => q.FilterExpression("is_deleted:false")
194+
.AggregationsExpression($"terms:(duplicate_signature~{maxResults} @min_count:2)"));
195+
196+
var buckets = result.Aggregations.Terms("terms_duplicate_signature")?.Buckets;
197+
if (buckets == null || buckets.Count == 0)
198+
return Array.Empty<string>();
199+
200+
return buckets.Select(b => b.Key).ToArray();
201+
}
202+
191203
protected override async Task AddDocumentsToCacheAsync(ICollection<FindHit<Stack>> findHits, ICommandOptions options, bool isDirtyRead)
192204
{
193205
await base.AddDocumentsToCacheAsync(findHits, options, isDirtyRead);

‎tests/Exceptionless.Tests/Jobs/CleanupDataJobTests.cs‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using Exceptionless.Core;
22
using Exceptionless.Core.Billing;
33
using Exceptionless.Core.Jobs;
4+
using Exceptionless.Core.Models;
45
using Exceptionless.Core.Repositories;
56
using Exceptionless.DateTimeExtensions;
67
using Exceptionless.Tests.Utility;
@@ -169,4 +170,152 @@ public async Task CanDeleteOrphanedEventsByStack()
169170
eventCount = await _eventRepository.CountAsync(o => o.IncludeSoftDeletes().ImmediateConsistency());
170171
Assert.Equal(5000, eventCount);
171172
}
173+
174+
[Fact]
175+
public async Task CanCleanupMultipleSoftDeletedOrganizations()
176+
{
177+
// Create more than the page size (5) of soft-deleted organizations to test pagination
178+
var orgs = new List<Organization>();
179+
for (int i = 0; i < 12; i++)
180+
{
181+
var org = _organizationData.GenerateSampleOrganization(_billingManager, _plans);
182+
org.Id = ObjectId.GenerateNewId().ToString();
183+
org.IsDeleted = true;
184+
orgs.Add(org);
185+
}
186+
await _organizationRepository.AddAsync(orgs, o => o.ImmediateConsistency());
187+
188+
// Create associated projects and events for a subset
189+
var project = _projectData.GenerateSampleProject();
190+
project.OrganizationId = orgs[0].Id;
191+
await _projectRepository.AddAsync(project, o => o.ImmediateConsistency());
192+
193+
var stack = _stackData.GenerateStack(generateId: true, organizationId: orgs[0].Id, projectId: project.Id);
194+
await _stackRepository.AddAsync(stack, o => o.ImmediateConsistency());
195+
196+
await _eventRepository.AddAsync(_eventData.GenerateEvents(10, orgs[0].Id, project.Id, stack.Id), o => o.ImmediateConsistency());
197+
198+
await _job.RunAsync(TestCancellationToken);
199+
200+
// All soft-deleted orgs should be removed
201+
foreach (var org in orgs)
202+
Assert.Null(await _organizationRepository.GetByIdAsync(org.Id, o => o.IncludeSoftDeletes()));
203+
204+
// Associated data should be gone too
205+
Assert.Null(await _projectRepository.GetByIdAsync(project.Id, o => o.IncludeSoftDeletes()));
206+
Assert.Null(await _stackRepository.GetByIdAsync(stack.Id, o => o.IncludeSoftDeletes()));
207+
var eventCount = await _eventRepository.CountAsync(o => o.IncludeSoftDeletes());
208+
Assert.Equal(0, eventCount);
209+
}
210+
211+
[Fact]
212+
public async Task CanEnforceRetentionForMultipleOrganizations()
213+
{
214+
// Retention enforcement uses GetBillingPlanByUpsellingRetentionPeriod which returns the next
215+
// plan with retention > org's retention. FreePlan (3d) → effective 30d, SmallPlan (30d) → effective 90d.
216+
var org1 = _organizationData.GenerateSampleOrganization(_billingManager, _plans);
217+
org1.Id = ObjectId.GenerateNewId().ToString();
218+
_billingManager.ApplyBillingPlan(org1, _plans.SmallPlan); // effective retention: 90 days (next plan up is Large)
219+
org1.StripeCustomerId = "cust_test1";
220+
org1.CardLast4 = "4242";
221+
org1.SubscribeDate = DateTime.UtcNow;
222+
org1.BillingChangedByUserId = TestConstants.UserId;
223+
224+
var org2 = _organizationData.GenerateSampleOrganization(_billingManager, _plans);
225+
org2.Id = ObjectId.GenerateNewId().ToString();
226+
_billingManager.ApplyBillingPlan(org2, _plans.FreePlan); // effective retention: 30 days (next plan up is Small)
227+
await _organizationRepository.AddAsync(new[] { org1, org2 }, o => o.ImmediateConsistency());
228+
229+
var project1 = _projectData.GenerateProject(generateId: true, organizationId: org1.Id);
230+
var project2 = _projectData.GenerateProject(generateId: true, organizationId: org2.Id);
231+
await _projectRepository.AddAsync(new[] { project1, project2 }, o => o.ImmediateConsistency());
232+
233+
var stack1 = _stackData.GenerateStack(generateId: true, organizationId: org1.Id, projectId: project1.Id);
234+
var stack2 = _stackData.GenerateStack(generateId: true, organizationId: org2.Id, projectId: project2.Id);
235+
await _stackRepository.AddAsync(new[] { stack1, stack2 }, o => o.ImmediateConsistency());
236+
237+
// Events inside retention for both (2 days old)
238+
var recentStart = DateTimeOffset.UtcNow.AddDays(-2);
239+
var recentEnd = DateTimeOffset.UtcNow.AddDays(-1);
240+
await _eventRepository.AddAsync(_eventData.GenerateEvents(10, org1.Id, project1.Id, stack1.Id, startDate: recentStart, endDate: recentEnd), o => o.ImmediateConsistency());
241+
await _eventRepository.AddAsync(_eventData.GenerateEvents(10, org2.Id, project2.Id, stack2.Id, startDate: recentStart, endDate: recentEnd), o => o.ImmediateConsistency());
242+
243+
// Events at 35 days old: outside org2's effective retention (30d) but inside org1's (90d)
244+
var olderStart = DateTimeOffset.UtcNow.AddDays(-37);
245+
var olderEnd = DateTimeOffset.UtcNow.AddDays(-33);
246+
await _eventRepository.AddAsync(_eventData.GenerateEvents(10, org1.Id, project1.Id, stack1.Id, startDate: olderStart, endDate: olderEnd), o => o.ImmediateConsistency());
247+
await _eventRepository.AddAsync(_eventData.GenerateEvents(10, org2.Id, project2.Id, stack2.Id, startDate: olderStart, endDate: olderEnd), o => o.ImmediateConsistency());
248+
249+
await _job.RunAsync(TestCancellationToken);
250+
await RefreshDataAsync();
251+
252+
// org1 should keep all events (within 90 day effective retention)
253+
var org1Events = await _eventRepository.CountAsync(q => q.FilterExpression($"organization:{org1.Id}"));
254+
Assert.Equal(20, org1Events);
255+
256+
// org2 should only keep recent events (older ones outside 30 day effective retention)
257+
var org2Events = await _eventRepository.CountAsync(q => q.FilterExpression($"organization:{org2.Id}"));
258+
Assert.Equal(10, org2Events);
259+
}
260+
261+
[Fact]
262+
public async Task CanCleanupSoftDeletedStacksAcrossPages()
263+
{
264+
var organization = await _organizationRepository.AddAsync(_organizationData.GenerateSampleOrganization(_billingManager, _plans), o => o.ImmediateConsistency());
265+
var project = await _projectRepository.AddAsync(_projectData.GenerateSampleProject(), o => o.ImmediateConsistency());
266+
267+
// Create more stacks than the page size (500) to test pagination
268+
var stacks = new List<Stack>();
269+
for (int i = 0; i < 600; i++)
270+
{
271+
var stack = _stackData.GenerateStack(generateId: true, organizationId: organization.Id, projectId: project.Id);
272+
stack.IsDeleted = true;
273+
stacks.Add(stack);
274+
}
275+
await _stackRepository.AddAsync(stacks, o => o.ImmediateConsistency());
276+
277+
// Create a valid non-deleted stack with events
278+
var validStack = await _stackRepository.AddAsync(_stackData.GenerateStack(generateId: true, organizationId: organization.Id, projectId: project.Id), o => o.ImmediateConsistency());
279+
await _eventRepository.AddAsync(_eventData.GenerateEvents(5, organization.Id, project.Id, validStack.Id), o => o.ImmediateConsistency());
280+
281+
await _job.RunAsync(TestCancellationToken);
282+
await RefreshDataAsync();
283+
284+
// All soft-deleted stacks should be removed
285+
var remainingStacks = await _stackRepository.CountAsync(o => o.IncludeSoftDeletes());
286+
Assert.Equal(1, remainingStacks);
287+
288+
// Valid events should remain
289+
var eventCount = await _eventRepository.CountAsync(o => o.IncludeSoftDeletes());
290+
Assert.Equal(5, eventCount);
291+
}
292+
293+
[Fact]
294+
public async Task RespectsMaxRetentionDays()
295+
{
296+
// FreePlan has 3 day retention, but enforcement uses GetBillingPlanByUpsellingRetentionPeriod
297+
// which returns the next plan up (SmallPlan, 30 days). So effective retention = 30 days.
298+
var organization = _organizationData.GenerateSampleOrganization(_billingManager, _plans);
299+
_billingManager.ApplyBillingPlan(organization, _plans.FreePlan);
300+
await _organizationRepository.AddAsync(organization, o => o.ImmediateConsistency());
301+
302+
var project = await _projectRepository.AddAsync(_projectData.GenerateSampleProject(), o => o.ImmediateConsistency());
303+
var stack = await _stackRepository.AddAsync(_stackData.GenerateSampleStack(), o => o.ImmediateConsistency());
304+
305+
// Events outside effective retention (30 days) should be deleted
306+
var outsideRetentionStart = DateTimeOffset.UtcNow.SubtractDays(37);
307+
var outsideRetentionEnd = DateTimeOffset.UtcNow.SubtractDays(33);
308+
await _eventRepository.AddAsync(_eventData.GenerateEvents(10, organization.Id, project.Id, stack.Id, startDate: outsideRetentionStart, endDate: outsideRetentionEnd), o => o.ImmediateConsistency());
309+
310+
// Events inside effective retention should be kept
311+
var insideRetentionStart = DateTimeOffset.UtcNow.SubtractDays(2);
312+
var insideRetentionEnd = DateTimeOffset.UtcNow.SubtractDays(1);
313+
await _eventRepository.AddAsync(_eventData.GenerateEvents(10, organization.Id, project.Id, stack.Id, startDate: insideRetentionStart, endDate: insideRetentionEnd), o => o.ImmediateConsistency());
314+
315+
await _job.RunAsync(TestCancellationToken);
316+
await RefreshDataAsync();
317+
318+
var eventCount = await _eventRepository.CountAsync(o => o.IncludeSoftDeletes());
319+
Assert.Equal(10, eventCount);
320+
}
172321
}

0 commit comments

Comments
 (0)