Reduce noisy change events - #2438
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b319fb57d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await _cache.SetAsync(GetTotalCacheKey(utcNow, organizationId), usage.Total, TimeSpan.FromHours(8)); | ||
| await _organizationRepository.SaveAsync(organization); | ||
| // Usage counters and last-event timestamps are operational updates, not user-facing entity changes. | ||
| await _organizationRepository.SaveAsync(organization, o => o.Notifications(false)); |
There was a problem hiding this comment.
When this usage save is the one that crosses the monthly or hourly limit, the backend can still emit PlanOverage, but the Svelte client does not refetch organization data for that message: src/Exceptionless.Web/ClientApp/src/routes/(app)/+layout.svelte only invalidates queries inside isEntityChangedType, and PlanOverage is not a *Changed entity message. Before this change, the accompanying OrganizationChanged from this SaveAsync refreshed is_over_monthly_limit / is_throttled; with Notifications(false), connected users can remain on stale organization state and miss the overage banner until another organization change or a reload. Please keep an organization invalidation for overage transitions or add a PlanOverage handler in the client.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f8449f0: the Svelte websocket handler now recognizes PlanOverage and invalidates only the affected organization, organization stats, and organization list queries. The endpoint also evaluates pending usage so the refetch observes the transition. Covered by the targeted frontend query-invalidation test and OrganizationEndpointTests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8449f0ae4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await _cache.SetAsync(GetTotalCacheKey(utcNow, organizationId), usage.Total, TimeSpan.FromHours(8)); | ||
| await _organizationRepository.SaveAsync(organization); | ||
| // Usage counters and last-event timestamps are operational updates, not user-facing entity changes. | ||
| await _organizationRepository.SaveAsync(organization, o => o.Notifications(false)); |
There was a problem hiding this comment.
When an organization hits the hourly bucket limit, the new PlanOverage handler refreshes the Svelte organization query to is_throttled: true, but this save is also where the completed bucket's throttled cache key is removed; with repository notifications disabled here, the previous OrganizationChanged refetch that cleared the hourly overage banner is no longer emitted. In an active session that does not remount or focus-refetch after the 5-minute throttle window, users can keep seeing the destructive hourly overage banner even though GetUsageAsync would now return IsThrottled = false; please keep a clear-side invalidation/notification for this state transition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 15e27df: hourly throttle transitions now write a per-bucket marker before PlanOverage is published. Saving that completed bucket emits one OrganizationChanged while routine usage saves remain silent. Covered by SavePendingUsageAsync_HourlyThrottleClears_PublishesOrganizationChangedMessage; the full UsageServiceTests suite passes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15e27df966
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await _cache.SetAsync(GetTotalCacheKey(utcNow, organizationId), usage.Total, TimeSpan.FromHours(8)); | ||
| await _organizationRepository.SaveAsync(organization); | ||
| // Routine usage updates stay silent, but clients need one refresh when an hourly throttle clears. | ||
| await _organizationRepository.SaveAsync(organization, o => o.Notifications(hourlyThrottleTransition?.Value ?? false)); |
There was a problem hiding this comment.
When an organization crosses the monthly limit from pending usage, this save no longer emits OrganizationChanged, leaving PlanOverage as the only websocket signal. The legacy Angular banner listens for PlanOverage (src/Exceptionless.Web/ClientApp.angular/components/organization-notifications/organization-notifications-directive.tpl.html:4), but its vm.get() path calls organizationService.getAll() with the default cached Restangular client (src/Exceptionless.Web/ClientApp.angular/components/organization/organization-service.js:94-99), and that cache is only cleared on OrganizationChanged/ProjectChanged (src/Exceptionless.Web/ClientApp.angular/components/organization/organization-service.js:11-12). The fresh evidence is this unchanged Angular cache path, so Angular users with a warm organization cache keep seeing stale is_over_monthly_limit until a later cache clear or reload; please either clear the Angular organization cache on PlanOverage or keep an organization invalidation for monthly overage transitions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9bd0f02: the legacy organization cache is now cleared on PlanOverage, so its notification refresh reads current overage state.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bd0f02c0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await _cache.SetAsync(GetTotalCacheKey(utcNow, organizationId), usage.Total, TimeSpan.FromHours(8)); | ||
| await _organizationRepository.SaveAsync(organization); | ||
| // Routine usage updates stay silent, but clients need one refresh when an hourly throttle clears. | ||
| await _organizationRepository.SaveAsync(organization, o => o.Notifications(hourlyThrottleTransition?.Value ?? false)); |
There was a problem hiding this comment.
When an organization is already over its monthly limit and the calendar rolls into a new month, the first usage save for the new month is a routine usage-only save with hourlyThrottleTransition false. Because this now calls Notifications(false), no OrganizationChanged is published, and there is no PlanOverage message for a clearing transition, so clients with cached is_over_monthly_limit: true can keep showing the monthly overage banner until a reload/focus refetch. Please keep a targeted organization notification or equivalent invalidation when monthly overage transitions from true to false.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in bf75993: the first usage save of a new month detects an over-limit previous-month record and emits one OrganizationChanged. Later buckets remain silent. The regression test failed before the fix with Expected 1, Actual 0; the full UsageServiceTests suite now passes 21 of 21.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf759932be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| currentUsage.Discarded = realTimeUsage.CurrentUsage.Discarded; | ||
| currentUsage.TooBig = realTimeUsage.CurrentUsage.TooBig; | ||
| currentUsage.Deleted = realTimeUsage.CurrentUsage.Deleted; | ||
| viewOrganization.IsOverMonthlyLimit = currentUsage.Limit >= 0 && currentUsage.Total >= currentUsage.Limit; |
There was a problem hiding this comment.
When an organization's temporary event bonus expires without another usage bucket being saved, realTimeUsage.CurrentUsage.Limit still contains the bonus-inflated limit written by BillingManager.ApplyBonus, while GetMaxEventsPerMonthWithBonus already returns the lower effective limit. This assignment therefore overrides the mapper's previously correct IsOverMonthlyLimit value and can leave the API response and overage banner false even when the persisted-plus-pending total now exceeds the live limit; compare the total against viewOrganization.GetMaxEventsPerMonthWithBonus(timeProvider) instead.
AGENTS.md reference: AGENTS.md:L67-L67
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2aa10d3: real-time usage totals are now compared against the organization view model live effective monthly limit, so an expired bonus cannot keep the overage state false. The focused regression failed before the fix with Expected True, Actual False; OrganizationEndpointTests now pass 103 of 103 and the build is clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2aa10d3966
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var previousMonthUtc = currentMonthUtc.AddMonths(-1); | ||
| bool hasCurrentMonthUsage = organization.Usage.Any(u => u.Date.Year == currentMonthUtc.Year && u.Date.Month == currentMonthUtc.Month); | ||
| var previousMonthUsage = organization.Usage.FirstOrDefault(u => u.Date.Year == previousMonthUtc.Year && u.Date.Month == previousMonthUtc.Month); | ||
| bool monthlyOverageCleared = !hasCurrentMonthUsage && previousMonthUsage is { Limit: > 0 } && previousMonthUsage.Total >= previousMonthUsage.Limit; |
There was a problem hiding this comment.
Fresh evidence is the bonus-expiration case: if the previous usage record retains a bonus-inflated Limit of 110, its total is 50, and the bonus expires so the live limit becomes 10, the API now correctly shows the organization as over limit, but this comparison remains false at rollover. Consequently the first new-month save suppresses OrganizationChanged, leaving connected clients' monthly-overage banner visible after the usage period has reset. Detect the prior overage using the effective live limit or persist an explicit transition marker rather than relying on the historical usage limit.
AGENTS.md reference: AGENTS.md:L72-L75
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 638388c: month-rollover detection now compares the previous month total with the organization’s live effective monthly limit, matching the API behavior after a bonus expires. The regression reproduced the issue before the fix (Expected 1, Actual 0); the full UsageServiceTests suite now passes 22 of 22 and the build is clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 638388c232
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await _cache.SetAsync(GetTotalCacheKey(utcNow, organizationId), usage.Total, TimeSpan.FromHours(8)); | ||
| await _organizationRepository.SaveAsync(organization); | ||
| // Routine usage updates stay silent, but clients need one refresh when an overage clears. | ||
| await _organizationRepository.SaveAsync(organization, o => o.Notifications((hourlyThrottleTransition?.Value ?? false) || monthlyOverageCleared)); |
There was a problem hiding this comment.
When a bonus expires while usage:limits:{id} still contains the bonus-inclusive value (cached for one day), GetEventsLeftAsync and IncrementTotalAsync continue using that stale limit and publish no PlanOverage, even if the live base limit has now been exceeded. This save recalculates effectiveMonthlyLimit, but notifications remain disabled because this is neither an hourly transition nor an overage-clear transition; the API-side recomputation only helps after a client already refetches. The fresh evidence beyond the resolved API issue is the one-day limit cache combined with this silent save, which leaves connected clients showing is_over_monthly_limit: false after the next usage bucket is persisted. Emit a targeted notification when the live monthly overage begins, or expire the cached limit when the bonus expires.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 22e8d0f: when a persisted bonus-inclusive limit becomes a lower live limit after that bonus expires, saving the bucket now publishes one monthly PlanOverage after the updated usage is persisted. The condition is scoped to the expired-bonus transition, so routine saves and plan changes do not duplicate it. The regression primes the stale limit cache and failed before the fix (Expected 1, Actual 0); all 23 UsageServiceTests now pass and the build is clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22e8d0f470
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| usage.Deleted += bucketDeleted?.Value ?? 0; | ||
| bool expiredBonusChangedLimit = organization.BonusEventsPerMonth > 0 | ||
| && organization.BonusExpiration <= _timeProvider.GetUtcNow().UtcDateTime | ||
| && recordedMonthlyLimit == organization.MaxEventsPerMonth + organization.BonusEventsPerMonth; |
There was a problem hiding this comment.
When the bonus-inclusive usage:limits cache was primed while the bonus was active but the organization has no persisted usage entry for the current month, events accepted after expiry create usage with the live base limit, so recordedMonthlyLimit already equals that base limit and this condition remains false. Because IncrementTotalAsync compared the same events against the stale bonus-inclusive cache, neither PlanOverage nor OrganizationChanged is published even when the bucket crosses the live limit, leaving connected clients unaware until another refresh. Fresh evidence beyond the resolved expired-bonus comment is the missing-current-month-record path; detect the transition independently of the historical usage limit and cover that legacy-record case.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9b95689: expired-bonus transition detection now uses the stale cached ingestion limit rather than requiring a persisted current-month usage record. When that cache is bonus-inflated, the save removes it, persists usage against the live limit, and publishes one monthly PlanOverage if the organization newly becomes over limit. Coverage now includes both persisted and missing usage records and verifies enforcement returns zero events left afterward; all 24 UsageServiceTests pass.
There was a problem hiding this comment.
Follow-up in 8b3a46c broadens the same transition guard to a legacy current-month record that already stores the live limit but was ingested against the stale bonus-inclusive cache. The cache is inspected only when the bucket moves from under its recorded limit to over the live expired-bonus limit. All three record shapes are covered; UsageServiceTests pass 25 of 25.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b3a46c600
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var cachedMonthlyLimit = await _cache.GetAsync<int>($"usage:limits:{organizationId}"); | ||
| expiredBonusChangedLimit = cachedMonthlyLimit.HasValue && cachedMonthlyLimit.Value > effectiveMonthlyLimit; |
There was a problem hiding this comment.
When the bonus-inclusive usage:limits entry expires after IncrementTotalAsync accepted events but before the completed bucket is saved, this lookup returns no value and expiredBonusChangedLimit remains false. The save then persists usage above the live post-bonus limit with notifications disabled, and later increments generally cannot publish PlanOverage because the total is already beyond the crossing point. This narrow end-of-cache-lifetime window therefore leaves connected clients unaware of the monthly overage; preserve the ingestion limit used by the bucket in a transition marker rather than relying on the one-day cache entry still existing at save time.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2392825: successful monthly PlanOverage publication now records a per-bucket transition marker. The usage save consumes that marker when evaluating an expired-bonus crossing, so it publishes only when ingestion did not already publish, independent of whether the one-day limit cache still exists. The save also clears the stale limit cache. Regressions cover cache expiry between increment/save and duplicate suppression; all 27 UsageServiceTests pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 239282518b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| message.change_type === ChangeType.Removed || | ||
| message.organization_id !== currentOrganizationId.current || | ||
| !message.project_id || | ||
| !projectNeedsConfiguration || |
There was a problem hiding this comment.
When a PersistentEventChanged message arrives while the projects query is still loading, projects is empty, so this guard discards the message instead of recording its project_id. If the initial query then returns the project with is_configured: false before the asynchronous SetProjectIsConfiguredWorkItem finishes, the setup notification remains visible until that job publishes ProjectChanged (or indefinitely if the job is delayed or fails), whereas the previous handler recorded the event regardless of query timing. Preserve valid IDs even when the project has not loaded yet.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 910499a: valid PersistentEventChanged project IDs are now recorded immediately for the current organization, without requiring the projects query to have loaded first. Once project data arrives, the local configured-ID set suppresses the stale setup notification without refetching organization or project data. Added a focused early-event regression; Svelte validation is clean and all 404 frontend unit tests pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 910499ab6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| await _organizationRepository.SaveAsync(organization); | ||
| await _organizationRepository.SaveAsync(organization, o => o.Notifications(false)); |
There was a problem hiding this comment.
When this repeatable migration repairs a current-month usage total from below the live limit to at or above it, Notifications(false) removes the OrganizationChanged signal that previously refreshed connected clients, while this path publishes no replacement PlanOverage. Users with warm Svelte or Angular organization caches therefore continue seeing the organization as under limit until an unrelated refresh; suppress routine migration notifications, but emit a targeted signal when the repaired current-month total crosses the effective limit.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ee08a86: the repeatable usage repair migration now detects only a current-month transition from below the live effective monthly limit to at or above it, saves the repaired usage silently, then publishes one monthly PlanOverage. Repeat runs remain silent because the persisted total is already repaired. The regression reproduced the issue before the fix (Expected 1, Actual 0), runs the migration twice to prove one-shot behavior, and both UpdateEventUsageMigrationTests plus all 27 UsageServiceTests pass; the build is clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee08a8612b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bool hasCurrentMonthUsage = organization.Usage.Any(u => u.Date.Year == currentMonthUtc.Year && u.Date.Month == currentMonthUtc.Month); | ||
| var previousMonthUsage = organization.Usage.FirstOrDefault(u => u.Date.Year == previousMonthUtc.Year && u.Date.Month == previousMonthUtc.Month); | ||
| bool monthlyOverageCleared = !hasCurrentMonthUsage && effectiveMonthlyLimit > 0 && previousMonthUsage?.Total >= effectiveMonthlyLimit; |
There was a problem hiding this comment.
When a below-limit current-month usage record already exists without a client notification—for example, UpdateEventUsage now creates the record from the current-month histogram and saves it with Notifications(false)—this presence check makes monthlyOverageCleared false. If connected clients still cache the previous month's overage state, this save and every later routine usage save remain silent, so the banner can persist indefinitely. Detect the transition from the previous month's overage independently of whether the current-month record exists, or persist an explicit clear marker.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed at the source in 740bba9: when UpdateEventUsage itself creates the first current-month usage record, it now detects an over-limit previous month followed by a repaired current-month total below the live effective limit and enables exactly one OrganizationChanged notification for that save. This preserves the existing one-shot behavior in UsageService and avoids turning every later routine usage save into another clear notification. The regression failed before the fix (Expected 1, Actual 0), runs the repeatable migration twice to verify the count stays at one, and all 3 migration tests plus all 27 UsageServiceTests pass; the build is clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 740bba9b8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bool monthlyOverageCleared = !hadCurrentMonthUsage | ||
| && effectiveMonthlyLimit > 0 | ||
| && previousMonthUsage?.Total >= effectiveMonthlyLimit | ||
| && currentMonthUsage?.Total < effectiveMonthlyLimit; |
There was a problem hiding this comment.
When this repeatable migration runs after a month rollover for an organization that was previously over limit but has no events or usage record in the new month, currentMonthUsage is null, so the nullable comparison evaluates false and the save suppresses OrganizationChanged. Connected clients can therefore retain the previous month's overage banner until an unrelated refetch, even though the new month's effective usage is zero; handle a missing current-month record as zero when detecting this clearing transition.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 29f3c50: missing current-month usage is now evaluated as zero for the rollover clear transition. When that clears an over-limit previous month, the migration persists a zero-usage current-month record before saving with one OrganizationChanged notification; that record is the durable marker that keeps subsequent repeatable runs silent. The focused regression failed before the fix (Expected 1, Actual 0), runs the migration twice, and verifies one notification plus the persisted zero record. All 4 migration tests and all 27 UsageServiceTests pass; the build is clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29f3c506fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await _cache.SetAsync(GetTotalCacheKey(utcNow, organizationId), usage.Total, TimeSpan.FromHours(8)); | ||
| await _organizationRepository.SaveAsync(organization); | ||
| // Routine usage updates stay silent, but clients need one refresh when an overage clears. | ||
| await _organizationRepository.SaveAsync(organization, o => o.Notifications((hourlyThrottleTransition?.Value ?? false) || monthlyOverageCleared)); |
There was a problem hiding this comment.
During a rolling deployment, a previous-version worker can set the hourly throttle key and publish PlanOverage without writing the newly introduced throttled-transition marker; if a new-version worker later persists that completed bucket, this condition disables OrganizationChanged while also removing the throttle key. A connected client that handled the overage then retains is_throttled: true until another refresh. Derive the transition from the completed bucket counters when the marker is missing, or otherwise provide compatibility for buckets created before this marker existed.
AGENTS.md reference: AGENTS.md:L72-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 068d20a: completed buckets now prefer the explicit hourly transition marker, but when it is absent they derive the legacy rolling-deployment transition from the persisted bucket total and the effective throttle threshold. The threshold is calculated at the completed bucket timestamp so month-boundary buckets retain the old worker’s semantics. The regression covers both marker-present and marker-missing cases; the missing case failed before the fix (Expected 1, Actual 0). All 28 UsageServiceTests and all 4 migration tests pass, and the build is clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 068d20af96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (monthlyOverageStarted) | ||
| await _messagePublisher.PublishAsync(new PlanOverage { OrganizationId = organization.Id }); |
There was a problem hiding this comment.
When the expired-bonus path reaches this call and PublishAsync fails transiently, the organization has already been saved above the live limit and the bucket counters, organization set, and transition marker have already been removed. Retrying SavePendingUsageAsync therefore cannot revisit this bucket, while later buckets see wasOverRecordedMonthlyLimit as true and will not publish the missing transition, so connected clients and notification workers can permanently miss the monthly overage. Keep durable transition state until publication succeeds or arrange a retryable/outbox-style publication.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 39e026f: organization buckets now remain discoverable until all organizations and required transitions complete. Monthly overage publication uses a per-bucket pending marker that survives counter cleanup and is removed only after PublishAsync succeeds, so a retry republishes without reapplying removed counters. The regression injects a publisher that fails once; before the fix the second save made only one attempt (Expected: 2, Actual: 1), and now it retries successfully. All 29 UsageServiceTests and all 4 migration tests pass; the build is clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39e026f75d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Routine usage updates stay silent, but clients need one refresh when an overage clears. | ||
| await _organizationRepository.SaveAsync(organization, o => o.Notifications(hourlyThrottleCleared || monthlyOverageCleared)); | ||
|
|
||
| await _cache.RemoveAllAsync(new[] { | ||
| GetBucketTotalCacheKey(bucketUtc, organizationId), |
There was a problem hiding this comment.
When SaveAsync succeeds but the following RemoveAllAsync fails, the organization-set key is now deliberately retained, so the next SavePendingUsageAsync rereads the same counters and executes the additive usage updates again. A transient cache failure after the Elasticsearch write can therefore permanently double-count the entire five-minute bucket in both monthly and hourly usage, potentially triggering false throttling or overage; record per-organization completion or otherwise make replay idempotent before retaining the bucket for retries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 555028a: each organization bucket now writes a pending persistence state containing the intended monthly and hourly totals before saving. If a retained bucket is retried, the service compares that state with the persisted organization and skips the additive mutations and notification save when the write already landed; the marker is removed only after cleanup and required publication complete. The regression injects a one-time RemoveAllAsync failure after the repository save, retries the bucket, and verifies both monthly and hourly totals remain 1 rather than being doubled. All 30 UsageServiceTests pass and the build is clean.
| await _organizationRepository.SaveAsync(organization, o => o.Notifications(monthlyOverageCleared)); | ||
| if (monthlyOverageStarted) | ||
| await _messagePublisher.PublishAsync(new PlanOverage { OrganizationId = organization.Id }); |
There was a problem hiding this comment.
When this PlanOverage publication fails transiently, the preceding save has already persisted the repaired total at or above the limit and the exception is only logged by the surrounding catch. On the next repeatable migration run, the total no longer crosses the threshold, so monthlyOverageStarted remains false and the message is never retried, permanently skipping both connected-client invalidation and the overage notification worker; persist a pending transition until publication succeeds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 555028a: UpdateEventUsage now writes a month-scoped pending overage-publication marker before saving the repaired organization, reads it on later repeatable runs, and removes it only after PlanOverage publishes successfully. A stale pending marker is discarded if the organization is no longer over the live effective limit. The regression uses a publisher that fails once, runs the migration twice, and verifies two publication attempts while the repaired total remains correct. All 5 migration tests pass and the build is clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 555028a8b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (monthlyOverageStarted) | ||
| await _cache.SetAsync(monthlyOveragePublicationPendingKey, true, TimeSpan.FromHours(8)); |
There was a problem hiding this comment.
When PlanOverage publication fails, the surrounding catch retains only this cache marker, but it expires after eight hours. MigrationJob is non-continuous and invokes the migration manager only once at startup (src/Exceptionless.Core/Jobs/Elastic/MigrationJob.cs:11,35), so the next production run may occur on a restart long after the marker has expired; because the repaired usage is already at the limit, that run no longer detects a crossing and never retries the message. The fresh evidence beyond the earlier publication-retry finding is the combination of this finite TTL and the migration's non-continuous schedule; retain the pending transition durably or arrange an immediate scheduled retry.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 31cfa5f: the month-scoped migration publication marker no longer expires. It remains in the shared cache until a later startup run either publishes PlanOverage successfully or determines that the organization is no longer over its live effective limit. The failure-injection test now verifies the marker exists with no expiration after the failed run and is removed after the successful retry. All 5 migration tests pass and the build is clean.
| bool usageAlreadyPersisted = usagePersistencePending is { HasValue: true } | ||
| && usagePersistencePending.Value.IsApplied(usage, hourlyUsage); | ||
| if (!usageAlreadyPersisted) |
There was a problem hiding this comment.
When a clear-side SaveAsync persists the organization but its OrganizationChanged publication fails transiently, the persistence marker remains and the retained bucket is retried. The freshly loaded usage then satisfies IsApplied, so this branch skips the only notification-producing save; cleanup subsequently removes the marker and bucket, leaving connected clients stuck with is_throttled or monthly-overage state until an unrelated refresh. Track clear-notification delivery separately and retry it even when the aggregate write is already applied.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 31cfa5f: clear delivery now has its own per-bucket pending marker written before the notification-producing save. If the aggregate write landed but OrganizationChanged publication failed, the persistence marker still prevents counter replay while the clear marker causes a notification-only retry; it is removed only after that save/publish completes. The regression fails the first BeforePublishEntityChanged attempt, retries successfully, verifies two delivery attempts, and confirms monthly/hourly totals were applied exactly once. All 31 UsageServiceTests pass and the build is clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31cfa5fb08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // This repeatable migration runs only at startup, so retain the retry marker until a later run publishes or invalidates it. | ||
| await _cache.SetAsync(monthlyOveragePublicationPendingKey, true); | ||
|
|
||
| await _organizationRepository.SaveAsync(organization, o => o.Notifications(monthlyOverageCleared)); |
There was a problem hiding this comment.
When this migration creates the first under-limit current-month usage record after an over-limit month, a transient EntityChanged publication failure from this notification-enabled SaveAsync is caught after Elasticsearch has persisted the record. On the next startup, hadCurrentMonthUsage is therefore true, so monthlyOverageCleared becomes false and the notification is never retried, leaving connected clients cached in the previous overage state. Unlike the normal usage path, this migration needs a pending clear-notification marker that is removed only after publication succeeds.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in cb541f7: the migration now writes a non-expiring, month-scoped clear-notification marker before the notification-enabled save, reads it on later startup runs, and removes it only after OrganizationChanged publication succeeds. The regression fails the first entity-change publication after persistence, verifies the marker remains without an expiration, then verifies the next run retries and clears it. While isolating that test, I also fixed the zero-event aggregation path so an absent histogram is treated as zero instead of returning before rollover-clear detection. Both zero-event tests now pass independently; all 6 migration tests, all 31 UsageServiceTests, and the build are green.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb541f786a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await _messagePublisher.PublishAsync(new PlanOverage { OrganizationId = organizationId }); | ||
| await _cache.SetAsync(GetMonthlyOverageTransitionKey(utcNow, organizationId), true, TimeSpan.FromHours(8)); |
There was a problem hiding this comment.
When an ordinary monthly-limit crossing reaches this block and PublishAsync fails transiently, the transition marker is never written; subsequent increments no longer satisfy the exact crossing condition, and the bucket save neither finds a pending publication nor emits the now-suppressed OrganizationChanged. Unlike the resolved expired-bonus save path, this normal IncrementTotalAsync path therefore permanently loses the PlanOverage notification and leaves connected clients stale until an unrelated refresh, so record a retryable publication marker before attempting the publish.
Useful? React with 👍 / 👎.
cb541f7 to
8895405
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e96f54f36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| _usageMonth = usageMonth; | ||
| $rootScope.$broadcast("OrganizationChanged"); |
There was a problem hiding this comment.
At UTC month rollover, this broadcasts OrganizationChanged without event data, but components/filter/filter-service.js:262-264 unconditionally dereferences organizationChanged.id. Every active legacy client therefore raises a TypeError during the synthetic monthly refresh; broadcast a compatible payload such as an empty object or update the listener to tolerate missing data.
Useful? React with 👍 / 👎.
|
1 participant
What changed
PlanOveragemessages without invalidating unrelated organization queries.PlanOverage.Why
Routine usage counters and persistent-event traffic were causing repeated organization and project reloads while the app was otherwise idle. The remaining notifications and refreshes are limited to state transitions and views that users need to see.
Verification
dotnet build tests/Exceptionless.Tests/Exceptionless.Tests.csproj --no-restore --disable-build-servers— passed with 0 warnings and 0 errorsUpdateEventUsageMigrationTests— 2 passedOrganizationEndpointTests— 103 passedUsageServiceTests— 22 passednpm run validate— passed with 0 errors and 0 warningsnpm run test:unit— 36 files / 406 tests passednpm run build— passednpm run build— passedBreaking changes
None.