Let a reminder be named for the item it waits on - #7
Conversation
Greptile SummaryThe PR adds independently keyed reminders by separating each alarm’s persisted name from the operation dispatched when it fires.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains. No blocking failure remains. Important Files Changed
Reviews (3): Last reviewed commit: "Bound a keyed reminder name by what the ..." | Re-trigger Greptile |
|
|
||
| return createStagedOperationMap(this.#operations, (operation, argumentsValue) => { | ||
| this.#intents.reminders.push({ | ||
| name: key === undefined ? operation : `${operation}:${key}`, |
There was a problem hiding this comment.
Composed reminder name exceeds storage
When a MySQL-backed actor schedules a reminder whose valid operation and key compose to more than 255 characters, the name exceeds reminders.operation, causing the turn-completion transaction to fail under strict mode or storing a truncated alarm name under permissive settings.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actor.ts
Line: 233
Comment:
**Composed reminder name exceeds storage**
When a MySQL-backed actor schedules a reminder whose valid operation and key compose to more than 255 characters, the name exceeds `reminders.operation`, causing the turn-completion transaction to fail under strict mode or storing a truncated alarm name under permissive settings.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| readonly actorId: string | ||
| /** Names the alarm. Without a key this is the operation. */ | ||
| readonly name: string | ||
| /** The message the alarm runs when it comes due. */ |
There was a problem hiding this comment.
Reminder comments duplicate the types
These field comments restate the adjacent name and operation semantics, and the same duplication appears on ReminderIntent.name, ReminderOptions.key, and schedule; removing the repeated prose reduces maintenance surface and prevents it from drifting from the API documentation.
Context Used: # De-AI Code Review
In this review, suggest simpl... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/reminder-administration.ts
Line: 9-12
Comment:
**Reminder comments duplicate the types**
These field comments restate the adjacent `name` and `operation` semantics, and the same duplication appears on `ReminderIntent.name`, `ReminderOptions.key`, and `schedule`; removing the repeated prose reduces maintenance surface and prevents it from drifting from the API documentation.
**Context Used:** # De-AI Code Review
In this review, suggest simpl... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Both findings are fixed in Composed reminder name exceeds storage (P1)Correct. The bound was on the key alone, so a long operation with a valid key composed a name past the 255 characters MySQL holds Under strict mode the turn-completion transaction fails. Under permissive settings the name is truncated, which is worse, because two alarms can then share a name and one silently takes the other's row. The check moved to the composed name, so a long operation with a short key is caught as well as the reverse. Regression test Reminder comments duplicate the types (P2)Fair. The comments on One thing I checked and did not addI started to guard against an operation containing the colon that separates a key, which would let an unkeyed This is the one place the two ports differ. The Ruby side needed the guard, because 245 tests, 0 failures. Typecheck, format, parameter-style and documentation checks all clean. |
A reminder is one alarm per actor and name, and the name was the operation, so an actor waiting on several things could hold only one alarm. Arming one per item kept the last and dropped the rest. schedule now takes a key. The key is the caller's own identifier for the item and names that item's alarm, so each item gets one, and scheduling the same key again moves that alarm and leaves the others alone. The operation column already named the alarm, so it keeps that job and a new message_operation carries what should run. It is null on existing rows, where the name is still the operation, so the migration adds a column and backfills nothing. Reminder administration now reports both: name is the alarm, operation is what it runs. Reporting the name as the operation would have been misleading for a keyed alarm.
The length was checked on the key alone, so a long operation with a valid key composed a name past the 255 characters MySQL holds the column in. Under strict mode the turn-completion transaction fails; under permissive settings the alarm identity is truncated, which is worse because two alarms can then share a name. It is checked on the composed name now, which catches a long operation with a short key as well as the reverse. An operation cannot hold the colon that separates a key, since actor member names are already restricted to letters, digits, and underscores, so a keyed name cannot collide with an unkeyed one and a key may hold colons of its own. The field comments restated their own names; what they carry now is only what the names do not say.
3df6a19 to
b87ef21
Compare
Adds
keytoschedule, so one actor can hold an alarm per item it is waiting on. This is the Node half of cardmagic/solid_objects#41.Why
A reminder is one alarm per actor and name, and the name was the operation. An actor waiting on several things could therefore hold only one alarm: arming one per item kept the last and dropped the rest, silently.
The workaround is to keep entries sorted, arm one alarm for the earliest, drain everything due when it fires, and re-arm. That works, and is still the better choice for a large queue of interchangeable items, but every actor holding scheduled work has to write it. In one application I have three actors doing exactly that, the third copied verbatim from the first.
What changes
Two entries now leave two reminders. Scheduling the same key again moves that item's alarm and leaves the others alone, so a keyed reminder is as safe to re-arm from a handler that may run twice as an unkeyed one.
Schema
The
operationcolumn already named the alarm, since the unique key is(instance_id, operation). It keeps that job, and a new nullablemessage_operationcarries what should actually run.That means the migration adds a column and backfills nothing: on an existing row
message_operationis null and the name is still the operation, which is exactly the old behaviour. Dispatch readsmessage_operation ?? operationin the two places that resolve a reminder to a message.Schema version 7. Following this repository's convention, the column is added by the migration rather than the base DDL, so a fresh install takes the same path.
Administration
reminders.all()now reports both:nameis the alarm,operationis what it runs. Previouslyoperationwas the only field and it was the name; for a keyed alarm that would have read as"deliver:item-7"and been misleading about what runs.Two existing tests rewrote the
operationcolumn to simulate a reminder whose message no longer exists. They now rewritemessage_operation, which is the column that decides dispatch. The behaviour they cover is unchanged; the column that expresses it moved.Differences from the Ruby PR
Ruby already had separate
nameandoperationcolumns, so it needed no migration. Node had one column doing both jobs, so it gainsmessage_operation. The public API is the same shape in both:keyon the schedule options, composed into the name asoperation:key, with the same 128-character bound.Testing
244 tests, 0 failures. Typecheck, format, parameter-style and documentation checks all clean.
New coverage: each key gets its own alarm; scheduling one key again moves only that alarm; an unkeyed alarm is still named for its operation; a keyed alarm runs the operation it was scheduled with; and both key bounds are refused.