From d30d95c91710ac54b5429bbb08e76aff764f1e10 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sun, 9 Aug 2026 12:23:55 +0000 Subject: [PATCH] fix(reminders): remove execution cap so reminders never skip on capacity (#1812 regression) The capacity gate (MaxConcurrentExecutions = 3) settled blocked occurrences instead of deferring them, and when the settlement ack raced with the cron reschedule it produced a bogus reminder.settlement.failed alert. Worse, it skipped real reminders (e.g. the daily vendor-price audit) when other reminders were parked on long-running executions. Every execution already has a one-hour absolute timeout and Akka.Reminders owns failure retry, so unbounded scheduling pressure on the LLM is acceptable. Remove the cap entirely - no capacity skip, no settle, no alert. --- .../specs/netclaw-scheduling/spec.md | 10 +++--- openspec/specs/netclaw-scheduling/spec.md | 19 +++++----- .../Reminders/ReminderManagerActorTests.cs | 36 +++++++++---------- .../Reminders/ReminderManagerActor.cs | 20 ----------- 4 files changed, 31 insertions(+), 54 deletions(-) diff --git a/openspec/changes/reliable-one-shot-reminder-retry/specs/netclaw-scheduling/spec.md b/openspec/changes/reliable-one-shot-reminder-retry/specs/netclaw-scheduling/spec.md index 3387f6bb6..d14fd0441 100644 --- a/openspec/changes/reliable-one-shot-reminder-retry/specs/netclaw-scheduling/spec.md +++ b/openspec/changes/reliable-one-shot-reminder-retry/specs/netclaw-scheduling/spec.md @@ -6,7 +6,7 @@ The reminder manager SHALL store consecutive failures in each reminder definitio The manager SHALL disable a reminder when the count reaches `FailurePauseThreshold`. The disabled definition SHALL remain available for status and diagnosis. -The manager SHALL enforce `MaxConcurrentExecutions`. It SHALL apply the bounded capacity policy when no execution slot is available. +The manager SHALL NOT cap the number of concurrent executions. Capacity was removed because every execution already has a one-hour absolute limit and Akka.Reminders owns failure retry, so unbounded scheduling pressure on the LLM is acceptable. Each execution SHALL have a one-hour absolute limit. A known timeout SHALL count as a failed attempt. @@ -24,12 +24,12 @@ Each execution SHALL have a one-hour absolute limit. A known timeout SHALL count - **WHEN** its next execution succeeds - **THEN** the manager saves a zero failure count -#### Scenario: The execution limit is full +#### Scenario: Reminder fires while other reminders are executing -- **GIVEN** `MaxConcurrentExecutions` reminder attempts are active +- **GIVEN** several reminder attempts are active - **WHEN** another occurrence arrives -- **THEN** the manager does not retain the envelope in a queue -- **AND** the manager applies the one-shot or reminder-series capacity policy +- **THEN** the manager starts the new execution immediately +- **AND** no occurrence is skipped or deferred for capacity reasons ### Requirement: Envelope-ack-gated at-least-once delivery for Mode B diff --git a/openspec/specs/netclaw-scheduling/spec.md b/openspec/specs/netclaw-scheduling/spec.md index a27192836..a9699f6dd 100644 --- a/openspec/specs/netclaw-scheduling/spec.md +++ b/openspec/specs/netclaw-scheduling/spec.md @@ -349,10 +349,11 @@ the library would mark an occurrence terminally failed. If either default changes in a way that breaks this ordering, add back a single operator knob. -The reminder manager SHALL enforce a maximum concurrent execution limit -(`MaxConcurrentExecutions`, internal const) and SHALL enforce a -per-execution timeout (`ExecutionTimeoutSeconds`, internal const on -`ReminderExecutionActor`). +The reminder manager SHALL allow any number of reminder executions to run +concurrently — there is no execution cap, because each execution already has a +one-hour absolute timeout and Akka.Reminders owns failure retry. The manager +SHALL enforce a per-execution timeout (`ExecutionTimeoutSeconds`, internal +const on `ReminderExecutionActor`). #### Scenario: Consecutive failures auto-pause task @@ -371,14 +372,12 @@ per-execution timeout (`ExecutionTimeoutSeconds`, internal const on - **THEN** the internal failure count for that reminder is reset to zero - **AND** subsequent failures start counting from zero again -#### Scenario: Max concurrent execution limit enforced +#### Scenario: Reminders run concurrently without an execution cap -- **GIVEN** `MaxConcurrentExecutions` is reached and that many reminders are currently executing +- **GIVEN** several reminders are already executing - **WHEN** another reminder fires -- **THEN** the new reminder is deferred to an internal queue -- **AND** the Akka.Reminders envelope is still acked (both Mode A and - Mode B deferred paths — a reminder that can't be dispatched yet is - acked and the library's retry/auto-pause machinery covers starvation) +- **THEN** the new reminder starts executing immediately +- **AND** no occurrence is skipped or deferred for capacity reasons #### Scenario: Execution timeout enforced diff --git a/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs b/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs index be3eeeff1..5561049f7 100644 --- a/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs +++ b/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs @@ -1035,7 +1035,7 @@ await AwaitAssertAsync(async () => } [Fact] - public async Task Recurring_occurrence_at_capacity_is_acked_without_execution() + public async Task Recurring_occurrence_starts_even_while_other_reminders_are_running() { var manager = await GetManagerAsync(); @@ -1051,18 +1051,19 @@ public async Task Recurring_occurrence_at_capacity_is_acked_without_execution() "auto-ack-capacity"); ActorRegistry.For(Sys).Register(autoAckRef); + // Fill three in-flight executions (the historical capacity limit). // Save before dispatch so filesystem latency cannot consume any test // timing window while execution slots are being filled. - for (var i = 0; i < ReminderManagerActor.MaxConcurrentExecutions; i++) + for (var i = 0; i < 3; i++) { var id = $"blocking-{i}"; _definitionStore.Save(CreateCurrentSessionDefinition(id, deliveryRequired: true)); } - for (var i = 0; i < ReminderManagerActor.MaxConcurrentExecutions; i++) + for (var i = 0; i < 3; i++) manager.Tell(CreateEnvelope($"blocking-{i}")); - for (var i = 0; i < ReminderManagerActor.MaxConcurrentExecutions; i++) + for (var i = 0; i < 3; i++) { var delivered = await gatewayProbe.ExpectMsgAsync( TimeSpan.FromSeconds(5), @@ -1073,17 +1074,19 @@ public async Task Recurring_occurrence_at_capacity_is_acked_without_execution() var invocationCount = _sessionPipeline.InvocationCount; var now = TimeProvider.System.GetUtcNow(); - var recurringId = "capacity-recurring"; + var recurringId = "concurrent-recurring"; var recurringReminder = new ReminderDefinition { Id = new ReminderId(recurringId), - Title = "Capacity recurring reminder", - Instructions = "Do not create a stale catch-up execution", + Title = "Concurrent recurring reminder", + Instructions = "Must start even while other reminders are executing", Delivery = new ReminderDelivery { Kind = DeliveryKind.None }, Schedule = new ReminderSchedule { Type = ReminderScheduleType.Interval, - Interval = TimeSpan.FromMinutes(30), + // Must exceed the 1h execution timeout + settlement margin, + // otherwise the safe-execution-lease check skips the occurrence. + Interval = TimeSpan.FromHours(2), FireAt = now.AddMilliseconds(100) }, Audience = TrustAudience.Team, @@ -1107,22 +1110,17 @@ await AwaitAssertAsync(async () => new GetReminderStatusQuery(recurringReminder.Id), TimeSpan.FromSeconds(3), TestContext.Current.CancellationToken); - Assert.Equal(1, status.SkippedDuplicates); - Assert.Equal(ReminderManagerActor.MaxConcurrentExecutions, - (await manager.Ask( - GetReminderHealthQuery.Instance, - TimeSpan.FromSeconds(3), - TestContext.Current.CancellationToken)).ActiveExecutions); + // The historical capacity gate would have skipped this occurrence + // (SkippedDuplicates == 1) without starting an execution. With the + // cap removed the occurrence must be dispatched, not skipped. + Assert.Equal(0, status.SkippedDuplicates); + Assert.True(_sessionPipeline.InvocationCount > invocationCount, + "Expected the recurring reminder to be executed by the pipeline."); }, duration: TimeSpan.FromSeconds(5), cancellationToken: TestContext.Current.CancellationToken); var stored = _definitionStore.Get(recurringReminder.Id); Assert.NotNull(stored); Assert.True(stored!.Enabled); - Assert.Equal(invocationCount, _sessionPipeline.InvocationCount); - - Assert.DoesNotContain(_notificationSink.Alerts, alert => - alert.Category == AlertType.ReminderExecutionFailed - && alert.Source == recurringId); } [Fact] diff --git a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs index 1faba717b..c26bd8474 100644 --- a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs +++ b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs @@ -24,12 +24,6 @@ public sealed partial class ReminderManagerActor : ReceiveActor public const string ShardRegionName = "netclaw-reminders"; public const string EntityId = "manager"; - /// - /// Maximum number of concurrent reminder executions. Not configurable — - /// if we ever need to tune this, add a knob then. - /// - internal const int MaxConcurrentExecutions = 3; - /// /// Consecutive execution failures after which a reminder is auto-paused. /// Not configurable. Must stay strictly below Akka.Reminders' @@ -609,20 +603,6 @@ await SettleBlockedOccurrenceAsync( return; } - if (_activeExecutions.Count >= MaxConcurrentExecutions) - { - _log.Info("Concurrency limit reached ({0}), settling blocked reminder '{1}'", - MaxConcurrentExecutions, reminderId.Value); - if (definition.Schedule.Type != ReminderScheduleType.OneShot) - RecordSkippedDuplicate(reminderId, definition.Title, "capacity"); - await SettleBlockedOccurrenceAsync( - definition, - envelope, - nack: definition.Schedule.Type == ReminderScheduleType.OneShot, - "Reminder execution capacity is unavailable."); - return; - } - if (!HasSafeExecutionLease(envelope, _timeProvider.GetUtcNow())) { var isOneShot = definition.Schedule.Type == ReminderScheduleType.OneShot;