docs: state that a reminder is one alarm per actor and name - #30
Conversation
Greptile SummaryThe PR documents reminder uniqueness and adds durable visibility when scheduling replaces an existing alarm.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (3): Last reviewed commit: "fix: start each turn attempt with empty ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| report_moved_reminder(reminder, previous_run_at) | ||
| reminder.save! |
There was a problem hiding this comment.
When the reminder save or a later write in the fenced transaction fails, report_moved_reminder has already synchronously published the replacement event, causing subscribers and structured logs to report an alarm move that was rolled back.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/solid_objects/executor.rb
Line: 236-237
Comment:
**Replacement event precedes commit**
When the reminder save or a later write in the fenced transaction fails, `report_moved_reminder` has already synchronously published the replacement event, causing subscribers and structured logs to report an alarm move that was rolled back.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Correct, and worth fixing precisely because of what the event is for. It exists to explain why an alarm did not fire. An event that can report a move a rolled back turn never made is worse than no event: it would send someone looking for a replacement that never happened.
Fixed in caab517. schedule_reminders now returns the moves instead of announcing them, and complete reports them after fenced_transaction returns. That is the same position the method already uses for effect.enqueued and broadcast.enqueued, so reminder moves now carry the same durability guarantee as every other event emitted from that turn rather than a weaker one.
New test "scheduling reports moves to its caller rather than announcing them" calls schedule_reminders directly and asserts it returns the move while emitting nothing. Verified: restoring the in-transaction emission fails it.
bundle exec rake: 388 runs, 1299 assertions, 0 failures, 0 errors, 14 skips.
@greptileai review
There was a problem hiding this comment.
The fix is exactly right and aligns with the existing pattern. A few things worth confirming:
moved_reminder_payload computes correctly before save!. It reads reminder.next_run_at from the in-memory model after assign_attributes, so the previous_run_at == reminder.next_run_at no-op guard is comparing the right values without needing a post-save reload.
filter_map is the right collector. The nil returns from moved_reminder_payload (new reminder, or same time) are dropped automatically, so moved_reminders contains only genuine moves.
Post-commit ordering matches the other events. moved_reminders.each fires after fenced_transaction returns, in the same position as broadcast.enqueued and effect.enqueued. The durability guarantee is now uniform across all three.
One minor pre-existing thing worth being aware of: moved_reminder_payload compares previous_run_at against reminder.next_run_at from the in-memory model, but the database may truncate sub-second precision on write. If the stored value differs from the in-memory value by a few microseconds, the "same time, report nothing" guard could produce a spurious event on the next move. This isn't introduced by this PR and the consequence is a harmless extra log line rather than a missing one — just worth knowing if you ever see unexpected reminder.replaced events with identical-looking timestamps.
LGTM.
|
@greptileai review |
| ) | ||
| enqueued_effects.concat(enqueue_effects(locked_message, instance, effect_intents)) | ||
| schedule_reminders(instance, reminder_intents) | ||
| moved_reminders.concat(schedule_reminders(instance, reminder_intents)) |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
@greptileai review |
schedule is keyed by actor and reminder name, so scheduling a name that is already armed moves that alarm instead of adding another. The behaviour is deliberate and matches Orleans and Durable Objects. It was stated nowhere, and it fails silently: an actor arming one reminder per queued item keeps only the last, and the earlier wake-ups never happen. A user shipped that to production and found it by hand. The reminders guide now gives the uniqueness key, shows the pattern that loses data, and shows the one-alarm-many-items pattern to use instead. That pattern is executed by a test rather than only read, since a sample nobody runs is how the next report gets written. Three tests pin the semantics being documented: a second schedule moves the alarm, distinct names coexist on one actor, and the same name on another actor is separate. A move that changes next_run_at now reports solid_objects.reminder.replaced carrying actor identity, name, and both times, with no arguments. Rescheduling to the same time reports nothing. The replacement was otherwise indistinguishable from a first schedule.
The event was emitted inside the fenced transaction and before the save, so a turn that rolled back would announce a move that never happened. An event whose only job is to explain why an alarm did not fire is worse than useless if it can report a move that was undone. Moves are collected during the transaction and reported after it, which is how the same method already reports enqueued effects and broadcasts.
The SQLite adapter retries the whole fenced transaction on a busy database, so the block can run more than once for one turn. Effects and moved reminders were accumulated in arrays declared outside the block, so an attempt that was rolled back left its entries behind and the committed attempt reported them again. The effect list has had that flaw since it was introduced; the reminder list inherited it. Both are cleared at the top of each attempt. No test. The harness runs inside a transactional test, so an inner rollback is a no-op and a stub that reruns the block produces a message-level retry rather than an adapter-level one, which measures something else. Rather than keep a test that passes for the wrong reason, the reasoning is recorded here.
0d3b35f to
21a5803
Compare
1 participant
Files the reported documentation gap, plus the optional visibility request.
Verified first
The report is accurate.
lib/solid_objects/executor.rbdoesReminder.find_or_initialize_by(instance:, name: intent.name)and thenassign_attributes+save!, and the schema backs it with a unique index on(instance_id, name). The reproduction is now a test: two entries, one reminder.The behaviour is right and stays as it is. It matches Orleans reminders and Durable Objects alarms, and it is what makes a reminder safe to re-arm from a handler that can run more than once. Only the documentation was missing.
1. The uniqueness key is stated
New README section,
Reminders > A reminder is one named alarm per actor, which says the key is(actor, reminder name), that a secondschedulemoves the existing alarm rather than adding one, and that the database enforces it. Also added todocs/architecture.md, and sharpened indocs/database-schema.mdwhere the index was described without saying what it means for a caller.It leads with the failure, using the reported shape:
It also states there is no
unschedule, since that is the next question someone asks.2. The one-alarm-many-items pattern is shown
The guide shows arming one alarm for the earliest item and draining everything due when it fires, then re-arming.
That sample is executed, not just printed.
RemindersTest#test_one_alarm_drains_every_due_item_and_arms_the_nextimplements it and asserts the due item was delivered, the later item is still pending, and the alarm was re-armed to the later time. A recommended pattern nobody runs is how the next report gets written.3. The replacement is now visible
solid_objects.reminder.replaced, carrying actor identity, remindername,previous_run_at, andnext_run_at. No arguments, matching how other events avoid application data.I used instrumentation rather than a bare debug log because it is the gem's existing idiom, and the bundled log subscriber already turns every
solid_objects.*event into a structured log line, so this shows up in logs without new machinery while also being subscribable. Rescheduling to the same time reports nothing, so a handler that re-arms itself at an unchanged time stays quiet.Tests
Four new tests pin the semantics being documented, plus the pattern test above:
schedulewith the same name moves the existing reminder (one row, later time)The first three passed before the code change, which is the point: they pin behaviour that was already correct and merely undocumented. Only the instrumentation test failed first.
Validation
bundle exec rake(SQLite)No version bump. The changelog entry sits under
Unreleased, matching the repository's separate release-prep workflow. Say the word if you want 0.10.3 cut from this instead.