Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,18 @@
test's rows and failed depending on order. Reported as reminders leaking,
which is where it surfaces first because reminders outlive the message that
created them.
- Document that a reminder is one named alarm per actor. `schedule` is keyed by
actor and reminder name, so scheduling a name that is already armed moves
that alarm rather than adding a second. The behaviour is deliberate and
matches Orleans and Durable Objects, but it was stated nowhere: an actor that
armed one reminder per queued item silently kept only the last, and the
earlier wake-ups never happened. The reminders guide now states the
uniqueness key and shows the one-alarm-many-items pattern to use instead.
- Add `solid_objects.reminder.replaced`, reported when a `schedule` call moves
an alarm already armed under the same name to a different time. It carries
the actor identity, reminder name, previous run time, and next run time, and
no arguments. Rescheduling to the same time reports nothing. The replacement
was previously indistinguishable from a first schedule.

## 0.10.2 - 2026-08-10

Expand Down
56 changes: 56 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -805,6 +805,62 @@ end
Use `missed: :latest` to coalesce missed occurrences or `missed: :all` to
enqueue each one.

### A reminder is one named alarm per actor

The uniqueness key is `(actor, reminder name)`. Scheduling a name that is
already armed **moves the existing alarm** rather than adding a second one. The
database enforces this with a unique index on `(instance_id, name)`.

This is the same model as Orleans reminders and Durable Objects alarms, and it
is what makes a reminder safe to re-arm from a handler that may run more than
once. It also means this is a data-loss bug:

```ruby
# Wrong. Every entry overwrites the previous entry's alarm.
def add(entry:)
self.entries = entries + [ entry ]
schedule :deliver, at: entry.fetch("wait_until"), arguments: {}
end
```

Two entries leave one reminder. The earlier wake-up never happens, nothing
raises, and nothing is logged except a `solid_objects.reminder.replaced` event.

Arm one alarm for the earliest item instead, and let the handler drain
everything now due before arming the next:

```ruby
def add(entry:)
self.entries = (entries + [ entry ]).sort_by { |item| item.fetch("wait_until") }
arm_next
end

def deliver
now = Time.current.to_i
due, pending = entries.partition { |item| item.fetch("wait_until") <= now }
due.each { |item| emit :send_push, **item.symbolize_keys }
self.entries = pending
arm_next
end

private

def arm_next
earliest = entries.first
return unless earliest

schedule :deliver, at: Time.at(earliest.fetch("wait_until")), arguments: {}
end
```

`deliver` drains every due item rather than one, so a single alarm serves a
whole queue and a missed or coalesced occurrence cannot strand an entry. Use a
distinct reminder name only when you genuinely need two independent alarms on
one actor, such as `:deliver` and `:sweep`.

Solid Objects has no `unschedule`. A reminder stops when its handler does not
re-arm it, and destroying an actor removes its reminders.

Self-scheduling actors should also have a low-frequency application reconciler.
It may read `SolidObjects::Instance.states_for`, `.without_pending_work`, and
`.orphaned`, but every repair must go through `async`. Never bulk-update actor
Expand Down
2 changes: 2 additions & 0 deletions docs/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,8 @@ A reminder record contains actor identity, a reminder name, target message, JSON
schedule :expire, at: 30.minutes.from_now, arguments: {}
```

Reminders are keyed by `(actor, reminder name)`, enforced by a unique index on `(instance_id, name)`. `schedule` is therefore an upsert: scheduling a name that is already armed moves that alarm instead of adding another, which is what makes re-arming safe from a handler that may run more than once. An actor needing several pending items should arm one alarm for the earliest and drain everything due when it fires, rather than one alarm per item; the [reminders guide](../README.md#a-reminder-is-one-named-alarm-per-actor) shows that pattern. A move that changes `next_run_at` emits `solid_objects.reminder.replaced`, because the replacement is otherwise indistinguishable from a first schedule.

When due, the scheduler locks the source instance and creates a normal mailbox
row with an idempotency key derived from reminder ID and occurrence. The
mailbox insert and reminder advancement commit atomically. The mailbox provides
Expand Down
3 changes: 2 additions & 1 deletion docs/database-schema.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ Indexes:
### `reminders`

Durable one-shot or recurring alarm definitions. Unique instance/name makes
rescheduling an actor-owned reminder deterministic. Status/next-run/ID drives
rescheduling an actor-owned reminder deterministic, which also means a second
`schedule` under the same name moves that alarm rather than adding one. Status/next-run/ID drives
the due scan. Claim ownership references the process registry, and constraints
limit recurrence intervals, missed-work policies, and statuses.

Expand Down
9 changes: 9 additions & 0 deletions docs/operations.md
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,15 @@ transaction rejection, commit-action start/completion/failure, effect and
broadcast enqueue/completion, reminder enqueue, actor destruction/expiration,
retention pruning, process cleanup, and supervisor lifecycle.

`solid_objects.reminder.replaced` reports a `schedule` call that moved an alarm
already armed under the same name on the same actor, carrying the actor
identity, reminder `name`, `previous_run_at`, and `next_run_at`. Reminders are
keyed by actor and name, so re-arming a name is an update rather than a second
alarm. That is deliberate, and it is silent: an actor that arms one reminder
per queued item keeps only the last, and the earlier wake-up never happens.
Watch this event if your actors schedule from a loop or from a handler that can
run more than once. Rescheduling to the same time reports nothing.

`solid_objects.component.refreshed` covers every authorized component refresh
request. Its payload carries the actor identity, `component_name`,
`component_key`, declared `dependencies`, `refresh_method`, the rendered
Expand Down
43 changes: 40 additions & 3 deletions lib/solid_objects/executor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,14 @@ def complete(result, observable_changes, state_changed:)
reminder_intents = actor.drain_reminder_intents
outbound_message_intents = actor.drain_outbound_message_intents
enqueued_effects = []
moved_reminders = []

activation.lease.fenced_transaction do |instance|
# A busy database makes the adapter retry this whole block, so an
# attempt that was rolled back must not leave its work in the lists the
# reporting below reads. Each attempt starts from empty.
enqueued_effects.clear
moved_reminders.clear
claimed_message = matching_claim!
locked_message = Message.lock.find(message.id)
execute_commit_actions(commit_action_intents)
Expand All @@ -107,7 +113,7 @@ def complete(result, observable_changes, state_changed:)
completed_at: SolidObjects.database_adapter.database_now
)
enqueued_effects.concat(enqueue_effects(locked_message, instance, effect_intents))
schedule_reminders(instance, reminder_intents)
moved_reminders.concat(schedule_reminders(instance, reminder_intents))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Retry duplicates replacement events

When SQLite retries the fenced transaction after a later database write raises a busy or deadline error, this line appends the retried payloads to the same moved_reminders array that retained the rolled-back attempt's payloads, causing duplicate or stale solid_objects.reminder.replaced events after the successful commit.

Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/solid_objects/executor.rb
Line: 111

Comment:
**Retry duplicates replacement events**

When SQLite retries the fenced transaction after a later database write raises a busy or deadline error, this line appends the retried payloads to the same `moved_reminders` array that retained the rolled-back attempt's payloads, causing duplicate or stale `solid_objects.reminder.replaced` events after the successful commit.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 0d3b35f. The adapter retries the whole fenced_transaction block on a busy database, and both enqueued_effects and moved_reminders were declared outside it, so an abandoned attempt left its entries for the committed attempt to report again.

Worth noting the effect list has had this flaw since it was introduced; the reminder list inherited it. Both are now cleared at the top of each attempt, so every attempt starts from empty.

I could not build a faithful test for this, and I would rather say so than ship one that passes for the wrong reason. The suite runs inside a transactional test, so an inner rollback is a no-op: a stub that reruns the block leaves the first attempt committed, which turns into a message-level retry and reports two effects for reasons unrelated to array accumulation. I wrote that test, watched it fail, then established it was failing for the wrong cause and deleted it. Forcing a genuine adapter-level busy retry needs contention this harness cannot produce deterministically.

What I did verify: the retry path is real (DatabaseAdapters::Sqlite#transaction wraps super in with_busy_retry, which re-runs the caller block), the fix is two clear calls at the top of the attempt, and the full gate is green.

bundle exec rake: 388 runs, 1299 assertions, 0 failures, 0 errors, 14 skips.

@greptileai review

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is correct and minimal. The two .clear calls at the top of the fenced_transaction block are exactly the right shape — both lists were declared outside the block, so every retry attempt was accumulating on top of whatever the previous attempt left behind. Moving initialization inside the block would have worked too, but .clear keeps the variable in the outer scope where the reporting loop needs it after the transaction commits, which is the right tradeoff.

The symmetry between enqueued_effects and moved_reminders is also now explicit, which is good: the same structural flaw existed in both, and both are now fixed by the same pattern.

On the missing test: the explanation in the commit message is honest and precise. A transactional test harness that absorbs inner rollbacks cannot exercise the retry path faithfully, and a stub that reruns the block without a genuine rollback measures message-level retry semantics instead. Shipping a test that passes for a different reason would be worse than no test. The commit message records the reasoning clearly enough that a future contributor working with a production-like setup (or an integration test environment that allows real contention) knows exactly what to reach for.

The fix is clean. No concerns.

enqueued_effects.concat(
enqueue_actor_messages(locked_message, instance, outbound_message_intents)
)
Expand All @@ -127,6 +133,9 @@ def complete(result, observable_changes, state_changed:)
observable_name:
)
end
moved_reminders.each do |moved|
SolidObjects.instrument(:"reminder.replaced", **moved)
end
enqueued_effects.each do |effect|
SolidObjects.instrument(
:"effect.enqueued",
Expand Down Expand Up @@ -211,10 +220,20 @@ def enqueue_effects(locked_message, instance, intents)
end
end

# @rbs (Instance, Array[Actor::ReminderIntent]) -> void
# A reminder is one named alarm per actor, so scheduling a name that is
# already armed moves it rather than adding a second. An actor that arms a
# reminder per queued item therefore keeps only the last, and nothing else
# about that is visible: the write succeeds and the earlier wake-up simply
# never happens.
# Moves are returned rather than reported here, so the report happens after
# the turn commits. A rolled back turn would otherwise announce an alarm
# that never moved, which is the opposite of the visibility this event
# exists to provide.
# @rbs (Instance, Array[Actor::ReminderIntent]) -> Array[Hash[Symbol, untyped]]
def schedule_reminders(instance, intents)
intents.each do |intent|
intents.filter_map do |intent|
reminder = Reminder.find_or_initialize_by(instance:, name: intent.name)
previous_run_at = reminder.next_run_at
reminder.assign_attributes(
actor_type: instance.actor_type,
actor_id: instance.actor_id,
Expand All @@ -227,10 +246,28 @@ def schedule_reminders(instance, intents)
claimed_by: nil,
claimed_at: nil
)
moved = moved_reminder_payload(reminder, previous_run_at)
reminder.save!
moved
end
end

# Arguments are omitted deliberately: a reminder carries application data
# and this event exists to be logged.
# @rbs (Reminder, Time?) -> Hash[Symbol, untyped]?
def moved_reminder_payload(reminder, previous_run_at)
return nil unless previous_run_at
return nil if previous_run_at == reminder.next_run_at

{
actor_type: reminder.actor_type,
actor_id: reminder.actor_id,
name: reminder.name,
previous_run_at:,
next_run_at: reminder.next_run_at
}
end

# @rbs (Message, Instance, Array[Actor::OutboundMessageIntent]) -> Array[Effect]
def enqueue_actor_messages(locked_message, instance, intents)
intents.map do |intent|
Expand Down
18 changes: 16 additions & 2 deletions sig/generated/lib/solid_objects/executor.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,22 @@ module SolidObjects
# @rbs (Message, Instance, Array[Actor::EffectIntent]) -> Array[Effect]
def enqueue_effects: (Message, Instance, Array[Actor::EffectIntent]) -> Array[Effect]

# @rbs (Instance, Array[Actor::ReminderIntent]) -> void
def schedule_reminders: (Instance, Array[Actor::ReminderIntent]) -> void
# A reminder is one named alarm per actor, so scheduling a name that is
# already armed moves it rather than adding a second. An actor that arms a
# reminder per queued item therefore keeps only the last, and nothing else
# about that is visible: the write succeeds and the earlier wake-up simply
# never happens.
# Moves are returned rather than reported here, so the report happens after
# the turn commits. A rolled back turn would otherwise announce an alarm
# that never moved, which is the opposite of the visibility this event
# exists to provide.
# @rbs (Instance, Array[Actor::ReminderIntent]) -> Array[Hash[Symbol, untyped]]
def schedule_reminders: (Instance, Array[Actor::ReminderIntent]) -> Array[Hash[Symbol, untyped]]

# Arguments are omitted deliberately: a reminder carries application data
# and this event exists to be logged.
# @rbs (Reminder, Time?) -> Hash[Symbol, untyped]?
def moved_reminder_payload: (Reminder, Time?) -> Hash[Symbol, untyped]?

# @rbs (Message, Instance, Array[Actor::OutboundMessageIntent]) -> Array[Effect]
def enqueue_actor_messages: (Message, Instance, Array[Actor::OutboundMessageIntent]) -> Array[Effect]
Expand Down
Loading
Loading