feat: cancel actor reminders - #54
Conversation
A recurring reminder could be started and never stopped. schedule() now returns a ReminderHandle, and unschedule() removes one alarm by operation, by operation and key, or by that handle. unscheduleAll() removes every key of one operation. Both cancellations are staged in the same list as the schedules, so they apply in the order the turn called them, commit with the state change that decided them, and cancel nothing when a turn throws. Cancellation works on both engines. The SQL repository deletes by the composed name, or by message_operation when every key goes. The Durable Objects store deletes by reminder name, and reads its rows to find the keys of one operation, which is cheap because one object holds one actor. The handle is a plain object, as emit already returns, so it serialises into actor state and still cancels after a deactivation. schedule() returned void before. transmit() keeps that contract through its own type, and the packaged consumer fixture now exercises the handle and both cancellations through the published surface.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Four defects from review. transmit() kept a public overload returning ScheduledOperationsFor, so callers saw the scheduling map after its calls started returning a handle. Only the implementation signature had changed, which callers never see. transmit() now has its own TransmittedOperationsFor, and both the type test and the packaged consumer fixture assert that its calls return nothing. unscheduleAll compared message_operation alone. That column is null on every row written before the keyed-reminder migration, where the name is still the operation, so an old recurring reminder survived the cancel and kept firing. It now compares COALESCE(message_operation, operation), which is the fallback the five dispatch sites already use. A cancellation could land between claimReminder and enqueueReminder. The reload then found no row and raised LostActivation, which is not UnknownOperation, so the scheduler rethrew and a supervised scheduler exited. enqueueReminder now separates a cancelled reminder from a lost claim: a row that is gone returns false and the occurrence is dropped quietly, while a row whose claim changed still raises. The two dispatches nested their cancellation branches inside an `in` check. ReminderIntent now carries an absent `cancel`, so the union discriminates on one property and both dispatches read as guard clauses. The contract said an occurrence already claimed still runs. That was wrong in the window this change defines: a cancellation cannot recall a message the scheduler already wrote, but it does pre-empt a claimed occurrence that has not been enqueued.
|
All four fixed in 3d93301. Three were real defects and I have added a regression test for each. 1. 2. One refinement to the suggestion: my first regression test nulled 3. The claim, cancel, enqueue window. Correct, and the consequence was worse than a dropped occurrence: I took your second option, cancellation winning, because that is what the Ruby port already does with 4. Nested conditionals. Fixed in both dispatches. The flat form did not type-check at first, because Validated: default suite 416 passed, Durable Objects 50, PostgreSQL 62, MySQL 51, plus |
|
@greptileai review |
|
@greptileai review |
The two failure outcomes nested inside one check. They read as guard clauses now, and the existence query still runs only when the reload found nothing, so a successful enqueue makes no extra read.
|
Issue 2 is fixed in e504508. The two outcomes read as guard clauses now, and the existence query still runs only when the reload found nothing, so a successful enqueue makes no extra read. Issues 1 and 3 were already fixed in 3d93301, the commit this review covers, so I think they carried over from the previous round rather than describing the current tree.
transmit<Keys extends keyof this, ActorType>(
this: Actor & Pick<this, Keys> & (Partial<ActorType> | NoInfer<this>),
): TransmittedOperationsFor<InferredActor<Keys, ActorType>>The public overload returns
Gates after this change: default suite 416 passed, Durable Objects 50, PostgreSQL 28, MySQL 24, plus |
|
@greptileai review |
1 similar comment
|
@greptileai review |
The reminder helper asserted an unknown instance into a fabricated shape and typed exec as Function, which threw away the row type. It takes the state the callback already supplies, as the neighbouring test does, and names the row type on exec. The typed operation reference still said both maps return void. A scheduled call returns a handle now and a transmitted one returns void, so it says that.
|
@greptileai review |
The example cancelled chaseCarrier from a Subscription that never declared or scheduled it, and used status, renewal, and MONTH without declaring them. It is two classes now, one per case, each declaring the operations it cancels.
schedule() already threw UnknownOperation for an operation the actor does not declare, because the staged operation map asserts it. unschedule() and unscheduleAll() took any string, composed a name from it, and deleted nothing. A typo cancelled quietly and left a recurring reminder running, which is the failure this feature exists to prevent. Both now check the operation against the actor's declared operations and throw the same error. A handle skips the check, because the schedule() call that produced it was already checked.
|
Added the operation-name check, and verified the whole feature end to end in a throwaway app. The gap. End to end. Two things a consumer meets that are worth knowing, neither a defect:
A direct call that fails reports Gates: default suite 417 passed, Durable Objects 50, plus |
|
@greptileai review |
reminder returns one armed alarm as a ScheduledReminder and reminders lists every key of one operation, matching what the Ruby port already offers. Both are async, because a TypeScript actor holds no rows and reads its own through a reader the runtime supplies at hydration. That is one injection point, so both engines get it: the SQL runtime reads the reminders table for the instance, and the Durable Objects engine reads the object's own store. A read starts from the committed rows and applies the intents staged so far, so an actor that schedules and then reads sees what the commit will write, and one that cancels and then reads sees the alarm gone. key and intervalMilliseconds are null rather than undefined. The first draft used undefined and a test caught it: returning a ScheduledReminder straight from an operation failed with InvalidPayload, because undefined does not serialise. Returning one is the obvious thing to do, so the type makes it work. A projection has no reader and throws rather than reporting an armed alarm as absent, which is the failure this whole feature exists to prevent. ScheduledReminder rather than ReminderStatus, because the administration API already exports that name for the status string.
|
Reading the schedule is in scope now, so this matches Ruby #75.
A read starts from the committed rows and applies the intents staged so far, so a schedule-then-read sees what the commit will write and a cancel-then-read sees the alarm gone. Two decisions worth recording.
The type is A projection has no reader and throws, rather than reporting an armed alarm as absent. That is the silent-wrong-answer failure the earlier rounds were all about. Verified: default suite 422 passed, Durable Objects 51, PostgreSQL 34, MySQL 30, plus Also re-ran the throwaway consumer app against a fresh |
|
@greptileai review |
| function reminderStatusOf(options: { | ||
| name: string | ||
| operation: string | ||
| runAtMilliseconds: number | ||
| intervalMilliseconds: number | null | ||
| missedPolicy: "all" | "latest" | ||
| status: ReminderStatus | ||
| }): ScheduledReminder { |
There was a problem hiding this comment.
The repository instruction says “Multiple small functions → inline if used once.” reminderStatusOf introduces a separate input shape and copies nearly every field unchanged at its only call site. Its reminderKeyOf helper is also used once. Constructing the value directly removes both layers and the duplicated type declaration.
Before:
view.set(
intent.name,
reminderStatusOf({
name: intent.name,
operation: intent.operation,
runAtMilliseconds: intent.atMilliseconds,
intervalMilliseconds: intent.intervalMilliseconds ?? null,
missedPolicy: intent.missedPolicy,
status: "scheduled",
}),
)After, removing both helper declarations:
view.set(intent.name, {
name: intent.name,
operation: intent.operation,
key:
intent.name === intent.operation
? null
: intent.name.slice(intent.operation.length + 1),
runAtMilliseconds: intent.atMilliseconds,
intervalMilliseconds: intent.intervalMilliseconds ?? null,
missedPolicy: intent.missedPolicy,
status: "scheduled",
handle: { name: intent.name },
})The same directive applies to handleName, which has one caller.
Before:
return handleName(operationOrHandle, options.key)After, removing handleName:
if (options.key !== undefined) {
throw new TypeError("a reminder handle already names its key")
}
const name = operationOrHandle?.name
if (typeof name !== "string" || name.length === 0) {
throw new TypeError("a reminder handle returned by schedule is required")
}
return namePreserve the composed-name explanation above reminderName when removing these helpers. This repository requirement must be satisfied before merging.
Context Used: # De-AI Code Review In this review, suggest simplifying this code by removing "AI slop": Remove: - Comments that restate what the code already shows - Docstrings that just repeat the function signature - Defensive code for impossible scenarios ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actor.ts
Line: 200-207
Comment:
**Single-use helpers add indirection**
The repository instruction says “Multiple small functions → inline if used once.” `reminderStatusOf` introduces a separate input shape and copies nearly every field unchanged at its only call site. Its `reminderKeyOf` helper is also used once. Constructing the value directly removes both layers and the duplicated type declaration.
Before:
```typescript
view.set(
intent.name,
reminderStatusOf({
name: intent.name,
operation: intent.operation,
runAtMilliseconds: intent.atMilliseconds,
intervalMilliseconds: intent.intervalMilliseconds ?? null,
missedPolicy: intent.missedPolicy,
status: "scheduled",
}),
)
```
After, removing both helper declarations:
```typescript
view.set(intent.name, {
name: intent.name,
operation: intent.operation,
key:
intent.name === intent.operation
? null
: intent.name.slice(intent.operation.length + 1),
runAtMilliseconds: intent.atMilliseconds,
intervalMilliseconds: intent.intervalMilliseconds ?? null,
missedPolicy: intent.missedPolicy,
status: "scheduled",
handle: { name: intent.name },
})
```
The same directive applies to `handleName`, which has one caller.
Before:
```typescript
return handleName(operationOrHandle, options.key)
```
After, removing `handleName`:
```typescript
if (options.key !== undefined) {
throw new TypeError("a reminder handle already names its key")
}
const name = operationOrHandle?.name
if (typeof name !== "string" || name.length === 0) {
throw new TypeError("a reminder handle returned by schedule is required")
}
return name
```
Preserve the composed-name explanation above `reminderName` when removing these helpers. This repository requirement must be satisfied before merging.
**Context Used:** # De-AI Code Review In this review, suggest simplifying this code by removing "AI slop": **Remove:** - Comments that restate what the code already shows - Docstrings that just repeat the function signature - Defensive code for impossible scenarios ... ([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!
There was a problem hiding this comment.
Fixed in 4c6a1a5. Both helpers are gone and the value is constructed at its only call site inside applyReminderIntent, including the key slice that reminderKeyOf held.
| if (intent.cancel === "all") { | ||
| for (const [name, status] of view) { | ||
| if (status.operation === intent.operation) view.delete(name) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
The cancel === "all" branch nests if (status.operation === intent.operation) inside its loop. This violates the repository directive to avoid nested conditionals when filtering or guard clauses can express the same logic.
Filter the matching entries first, then delete them in a loop without an inner conditional. This preserves the early return and separates selection from deletion. The repository requirement must be satisfied before merging.
| if (intent.cancel === "all") { | |
| for (const [name, status] of view) { | |
| if (status.operation === intent.operation) view.delete(name) | |
| } | |
| return | |
| } | |
| if (intent.cancel === "all") { | |
| const matching = [...view].filter( | |
| ([, status]) => status.operation === intent.operation, | |
| ) | |
| for (const [name] of matching) view.delete(name) | |
| return | |
| } |
Context Used: # De-AI Code Review In this review, suggest simplifying this code by removing "AI slop": Remove: - Comments that restate what the code already shows - Docstrings that just repeat the function signature - Defensive code for impossible scenarios ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actor.ts
Line: 221-226
Comment:
**Cancellation adds avoidable nesting**
The `cancel === "all"` branch nests `if (status.operation === intent.operation)` inside its loop. This violates the repository directive to avoid nested conditionals when filtering or guard clauses can express the same logic.
Filter the matching entries first, then delete them in a loop without an inner conditional. This preserves the early return and separates selection from deletion. The repository requirement must be satisfied before merging.
```suggestion
if (intent.cancel === "all") {
const matching = [...view].filter(
([, status]) => status.operation === intent.operation,
)
for (const [name] of matching) view.delete(name)
return
}
```
**Context Used:** # De-AI Code Review In this review, suggest simplifying this code by removing "AI slop": **Remove:** - Comments that restate what the code already shows - Docstrings that just repeat the function signature - Defensive code for impossible scenarios ... ([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!
There was a problem hiding this comment.
Fixed in 3d93301. unschedule() is four lines with no branches at all now, because the name resolution moved into #reminderNameOf, which returns early on the string case. The Cloudflare dispatch is two flat if blocks ending in continue.
One thing worth recording: the flat form did not type-check at first, because "cancel" in intent was doing the discrimination. ReminderIntent carries an absent cancel now, so the union discriminates on one property.
A projection hydrated its actor without a reader, so an observable that asked what was armed threw instead of answering. The Ruby port reads from an observable, and a test there covers it, so this side did the same. Both Durable Objects projections and the SQL snapshot projection now pass the reader they already have the instance for. Record the one difference that stays. ScheduledReminder carries no occurrence count, because the SQL backends track one and Durable Objects does not, and reporting it for one backend only would be worse than leaving it out.
|
Audited the two implementations against each other after adding reading. Three gaps, one real bug on this side, now fixed. A projection had no reader, so an observable that asked what was armed threw. The Ruby port reads from an observable and has a test for it, so this was a scope gap rather than a deliberate limit. Both Durable Objects projections and the SQL snapshot projection now pass the reader they already have the instance for, and a test covers reading through a snapshot. Operation validation in the readers was already correct here; the Ruby side was missing it and is fixed there. One difference stays, and it is principled. Ruby's The remaining differences are language shape rather than scope: reading is async here because a TypeScript actor holds no rows, and the field names and units follow each language's conventions, Gates: default suite 423 passed, Durable Objects 51, plus |
|
@greptileai review |
A one-shot keeps its row after it fires, as completed, and the view returned it. A next-run lookup reported an old time rather than nothing, and an existence check refused to re-arm an alarm that could never fire again. The shared view now drops completed rows, so both engines agree without either having to remember. buildSnapshotWithIncarnation hydrated without a reader, so a snapshot getter that read its schedule threw. The earlier test missed it because it used an operation rather than a getter, so the read ran in an ordinary turn. It is a getter now and asserts through the snapshot. Inline the two single-use helpers that built a ScheduledReminder, and replace the double assertion in the malformed-handle test with an expected type error.
|
All four fixed in 4c6a1a5. Two were real defects and your critique of my test was correct. Spent one-shot reported as armed. Correct, and it was in both ports. The filter is in the shared view rather than in each engine, so a future backend cannot forget it. Paused rows stay, because a paused reminder still exists and
The double assertion is replaced with The two single-use helpers are inlined; the value is constructed at its only call site. Gates: default suite 424 passed, Durable Objects 51, PostgreSQL 36, MySQL 32, plus |
|
@greptileai review |
1 similar comment
|
@greptileai review |
The Ruby port covers a read from on_activate and this side did not. The test starts a runtime, arms an alarm, closes it, and starts a second one against the same file, so onActivate runs again with the reminder already armed. It fails when the turn reader is removed.
1 participant
The JavaScript counterpart of
cardmagic/solid-objects-ruby#75.
Why
schedule()acceptseveryMilliseconds, so an actor can start a repeatingalarm. Nothing stopped one. A search of
src/forunschedulereturned nomethod, so the only way to stop a recurring reminder was to edit storage by hand
or destroy the actor.
What changed
this.unschedule(this.renewal)andthis.unschedule("chargeRenewal")cancel thesame alarm. Prefer the handle when the actor already stored one, because it
cannot drift from the name that armed the reminder.
A keyed alarm cancels by the key that armed it, and
unscheduleAll()cancelsevery key of one operation:
Cancellations are staged in the same list as the schedules, so they apply in the
order the turn called them, commit with the state change that decided them, and
cancel nothing when a turn throws. Cancelling an alarm that does not exist is not
an error.
unschedule()andunscheduleAll()refuse an operation the actor does notdeclare, throwing the
UnknownOperationthatschedule()already throws. A typocancelled nothing quietly before, which is the failure this feature exists to
prevent. A handle skips that check, because the
schedule()call that producedit was already checked.
unschedule()returns nothing. It stages an intent rather than applying one, soan answer given at call time could be stale by the time the turn commits.
Both engines
The SQL repository deletes by the composed name, and by
message_operationwhenevery key goes, so
unscheduleAllneeds no prefix match.The Durable Objects engine needed the same work, which the type checker caught
rather than a test:
src/cloudflare/engine.tsalso consumesintents.reminders. Its store deletes by reminder name, and reads its rows tofind the keys of one operation, which is cheap because one object holds one
actor.
The handle
EffectHandleis already{ id: string }, a plain object that serialises intoactor state.
ReminderHandleis{ name: string }and gets the same treatment,so a handle stored in state still cancels after a deactivation. Orleans
documents that its own reminder handle cannot survive an activation, which is
why that API cancels by name alone. Here the handle is a value, not a
registration.
Passing a handle together with
keyis aTypeError, because the handle alreadynames the key.
Behaviour change
schedule(...).operation()returnedvoid. It now returnsReminderHandle.This is the change
emitmade in 0.15.0.transmit()keeps the old contractthrough its own
StagedOperationMaptype, so only scheduling is affected.Two fixtures asserted the old contract and now assert the new one:
test/actor-operations.types.tsandtest/fixtures/actor-operations-consumer.mts. The consumer fixture is thestronger one, because it exercises the handle and both cancellations through the
published package surface rather than through
src/.Reading the schedule
reminder()returns one armed alarm andreminders()lists every key of oneoperation, matching the Ruby port:
Both are async, because a TypeScript
Actorholds no rows. It reads its ownthrough a reader the runtime supplies at hydration, which is one injection point
for both engines: the SQL runtime reads the reminders table for the instance, and
the Durable Objects engine reads the object's own store.
A read starts from the committed rows and applies the intents staged so far, so
an actor that schedules and then reads sees what the commit will write, and one
that cancels and then reads sees the alarm gone.
keyandintervalMillisecondsarenullrather thanundefined. The firstdraft used
undefinedand a test caught it: returning aScheduledReminderstraight from an operation failed with
InvalidPayload, becauseundefineddoesnot serialise. Returning one is the obvious thing to do, so the type makes it
work.
A projection has no reader and throws rather than reporting an armed alarm as
absent, which is the failure this feature exists to prevent.
The type is
ScheduledReminder, notReminderStatus, because the administrationAPI already exports that name for the status string.
Tests
test/reminder-cancellation.test.tsis new, 10 tests. 9 of the 10 fail withsrc/reverted tomain, confirmed by reverting.Covered: the handle names the reminder; cancel by name; cancel by handle; a
recurring reminder stops and the scheduler then finds nothing due; cancelling an
absent reminder is not an error; a failed turn cancels nothing; cancel then
schedule in one turn leaves the new time; a malformed handle is rejected; a keyed
cancel spares its siblings;
unscheduleAllspares other operations.test/cloudflare/runtime.test.tsgains two Durable Objects tests: a cancelledalarm never fires, and keyed cancellation followed by
unscheduleAllempties thereminder table.
Validation
format:checkcheckbuildtest:packagetest:recoverySkip counts are unchanged from
main; the suite grew by the 10 new tests.Compatibility
No migration. New exports
ReminderHandle,ReminderMutation,UnscheduleIntent, andUnscheduleAllIntent, all documented indocs/api.mdas the documentation check requires.