fix(reminders): retain failed one-shot executions - #1812
Conversation
| if (_activeExecutions.TryGet(reminderId, out var activeExecution)) | ||
| { | ||
| RecordSkippedDuplicate(reminderId, definition.Title, "active"); | ||
| if (IsSameDeliveryAttempt(activeExecution.Envelope, envelope)) |
There was a problem hiding this comment.
An exact redelivery has the same key, due time, and deadline. The active child still owns settlement, so this handler sends no Ack or Nack.
| <AkkaHostingVersion>1.5.70</AkkaHostingVersion> | ||
| <AkkaPersistenceSqlHostingVersion>1.5.70</AkkaPersistenceSqlHostingVersion> | ||
| <AkkaRemindersVersion>0.6.0</AkkaRemindersVersion> | ||
| <AkkaRemindersVersion>0.7.0</AkkaRemindersVersion> |
There was a problem hiding this comment.
Akka.Reminders 0.7.0 supplies NackAsync and the durable occurrence-status query. Netclaw uses both APIs for retry control and reconciliation.
| /// Number of consecutive failed execution attempts for this reminder. | ||
| /// A successful attempt resets this value. | ||
| /// </summary> | ||
| public int ConsecutiveFailures { get; set; } |
There was a problem hiding this comment.
These fields use the existing JSON reminder definition. Missing fields keep safe defaults, so old definitions load without a database migration.
| return new ReminderStateResponse(id, Found: false, Enabled: false, ErrorMessage: "Reminder not found."); | ||
|
|
||
| definition = definition with | ||
| var candidate = definition with |
There was a problem hiding this comment.
This candidate prevents a failed enable request from erasing stored diagnostics. The store changes only after Akka.Reminders accepts the schedule.
| await SettleBlockedOccurrenceAsync( | ||
| definition, | ||
| envelope, | ||
| nack: definition.Schedule.Type == ReminderScheduleType.OneShot || sameOccurrence, |
There was a problem hiding this comment.
A one-shot or the same occurrence receives a Nack for retry. A later recurring occurrence receives an Ack and is skipped.
| await SettleBlockedOccurrenceAsync( | ||
| definition, | ||
| envelope, | ||
| nack: definition.Schedule.Type == ReminderScheduleType.OneShot, |
There was a problem hiding this comment.
Netclaw does not keep a deferred queue. A blocked one-shot returns to Akka.Reminders, while a recurring occurrence is skipped.
| ReminderEnvelope<ReminderPayload> envelope, | ||
| DateTimeOffset now) => | ||
| envelope.Deadline.IsInfinite | ||
| || envelope.Deadline.UtcDateTime - now >= ReminderExecutionActor.ExecutionAttemptTimeout + SettlementMargin; |
There was a problem hiding this comment.
The Ack lease must contain the one-hour attempt and the settlement margin. This guard rejects work that cannot settle safely.
| finally | ||
| { | ||
| _activeExecutions.TryRemove(outcome.Id, outcome.ExecutionId, out _); | ||
| replyTo.Tell(new ReminderExecutionAccepted(outcome.ExecutionId)); |
There was a problem hiding this comment.
This reply completes the settlement handshake. The child stays alive until the manager handles persistence and Ack or Nack.
| private async Task HandleExecutionTerminatedAsync(ReminderExecutionTerminated terminated) | ||
| { | ||
| if (!_activeExecutions.Remove(completed.Id)) | ||
| if (!_activeExecutions.TryRemove(terminated.Id, terminated.ExecutionId, out var execution)) |
There was a problem hiding this comment.
DeathWatch covers a child that exits before the handshake. The execution ID prevents a stale notice from settling a newer attempt.
| { | ||
| try | ||
| { | ||
| _definitionStore.Save(definition with |
There was a problem hiding this comment.
The manager writes local success state before external settlement. This order prevents an Ack from hiding a later definition-store failure.
| ["title"] = title, | ||
| ["failureCount"] = count.ToString(), | ||
| })); | ||
| _definitionStore.Save(definition with |
There was a problem hiding this comment.
The one-shot becomes disabled only after Ack returns success. Its definition and history remain available for status queries.
| TerminalOutcome = thresholdReached ? ReminderTerminalOutcome.Failed : definition.TerminalOutcome, | ||
| UpdatedAtMs = _timeProvider.GetUtcNow().ToUnixTimeMilliseconds() | ||
| }; | ||
| _definitionStore.Save(definition); |
There was a problem hiding this comment.
The manager saves the failed attempt before it sends the Nack. This preserves diagnostics if Akka.Reminders settlement fails.
| EmitSettlementFailure(definition, ex.Message, ex); | ||
| } | ||
|
|
||
| var occurrenceTerminal = nack?.ResponseCode is ReminderNackResponseCode.Failed |
There was a problem hiding this comment.
Failed and Expired mean that the occurrence exhausted its Akka.Reminders budget. Netclaw then disables the definition.
| { | ||
| await DeleteReminderInternalAsync(definition.Id); | ||
| deletedOneShots++; | ||
| var occurrence = await GetOccurrenceStatusAsync(definition); |
There was a problem hiding this comment.
Reconciliation now checks the durable occurrence result. A past due time and an absent active schedule no longer prove success.
| var schedulesTask = ListScheduledRemindersAsync(); | ||
| var historyTask = _historyStore.ReadAsync(query.Id, RecentHistoryCount); | ||
| await Task.WhenAll(schedulesTask, historyTask); | ||
| var occurrenceTask = GetOccurrenceStatusAsync(definition); |
There was a problem hiding this comment.
Status combines the Netclaw definition, recent history, and the Akka.Reminders occurrence. Operators can distinguish retries from terminal failure.
|
|
||
| // Drain stream stages before stopping so they complete gracefully | ||
| // rather than being abruptly terminated as actor children. | ||
| RunTask(async () => |
There was a problem hiding this comment.
The child drains and stops only after manager acceptance. This handshake makes DeathWatch a fallback for an unexpected exit.
| { | ||
| reminders.WithSettings(new ReminderSettings | ||
| { | ||
| AckTimeout = ReminderAckTimeout |
There was a problem hiding this comment.
The 70-minute lease exceeds the one-hour execution limit. The manager reserves one additional minute before it starts an attempt.
| _executionId, | ||
| _definition.Id, | ||
| success, | ||
| history, |
There was a problem hiding this comment.
The child reports history and outcome but sends no Ack or Nack. The singleton manager owns every settlement decision.
Aaronontheweb
left a comment
There was a problem hiding this comment.
LGTM - no more hard deletes of completed reminders.
| The retry uses bounded backoff and the same durable occurrence identity. A | ||
| successful attempt resets the consecutive failure count. | ||
|
|
||
| A one-shot reminder stays enabled while an occurrence can retry. A successful |
| var loaded = new ReminderDefinitionStore(_paths).Get(definition.Id); | ||
|
|
||
| Assert.NotNull(loaded); | ||
| Assert.Equal(5, loaded.ConsecutiveFailures); |
There was a problem hiding this comment.
Demonstrates that we save the failure execution data
| } | ||
|
|
||
| [Fact] | ||
| public void Definition_without_failure_fields_loads_with_active_defaults() |
There was a problem hiding this comment.
backwards compat test
| } | ||
|
|
||
| [Fact] | ||
| public async Task Execution_waits_for_manager_acceptance_before_stop() |
There was a problem hiding this comment.
Removed the duplicate SourceWithQueue Akka.Streams setup here - not necessary to have.
| } | ||
|
|
||
| internal sealed record ActiveReminderExecution( | ||
| Guid ExecutionId, |
There was a problem hiding this comment.
set by the ReminderManagerActor when execution begins
| _deliveryTimeoutCancelable?.Cancel(); | ||
| _deliveryTimeoutCancelable = null; | ||
|
|
||
| if (_pendingHistory is not null) |
There was a problem hiding this comment.
Execution history is now owned by the ReminderManagerActor and not the ReminderExecutionActor
| private IReminderClient? _client; | ||
|
|
||
| private readonly ActiveExecutionTracker _activeExecutions = new(); | ||
| private readonly Queue<ReminderId> _deferredQueue = new(); |
There was a problem hiding this comment.
Essentially, we were competing with Akka.Reminders' own queue management for reminder deliveries here. We are now delegating more of that TCO to Akka.Reminders directly now.
| AkkaReminderProtocol.ReminderNackResponse? nack = null; | ||
| try | ||
| { | ||
| nack = await _client!.NackAsync(execution.Envelope, reason); |
There was a problem hiding this comment.
proactively let Akka.Reminders know that this failed reminder needs re-delivery
…ity (#1812 regression) (#1839) 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.
Summary
Dependency
Validation
Closes #1803