Skip to content

fix(rivetkit): clean up cron history efficiently#5427

Merged
NathanFlurry merged 5 commits into
mainfrom
cron
Jul 20, 2026
Merged

fix(rivetkit): clean up cron history efficiently#5427
NathanFlurry merged 5 commits into
mainfrom
cron

Conversation

@NathanFlurry

@NathanFlurry NathanFlurry commented Jul 19, 2026

Copy link
Copy Markdown
Member

No description provided.

@railway-app

railway-app Bot commented Jul 19, 2026

Copy link
Copy Markdown

🚅 Deployed to the rivet-pr-5427 environment in rivet-frontend

Service Status Web Updated (UTC)
kitchen-sink 😴 Sleeping (View Logs) Web Jul 20, 2026 at 6:57 am
website 😴 Sleeping (View Logs) Web Jul 19, 2026 at 6:05 pm
frontend-inspector 😴 Sleeping (View Logs) Web Jul 19, 2026 at 6:03 pm
frontend-cloud 😴 Sleeping (View Logs) Web Jul 19, 2026 at 6:02 pm
mcp-hub ✅ Success (View Logs) Web Jul 19, 2026 at 5:57 pm
ladle ✅ Success (View Logs) Web Jul 19, 2026 at 5:57 pm

@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5427 July 19, 2026 12:57 Destroyed
@claude

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Review

Reviewed the core Rust scheduling logic, the vbare inspector protocol bump (v6), NAPI/WASM bindings + the Rust rivetkit wrapper, and the frontend schedules dashboard. Verified findings against file contents at the PR head commit and against release history (gh api repos/rivet-dev/rivet/releases).

Correctness: schedules feature-gate uses an already-released version number

frontend/src/components/actors/actor-inspector-context.tsx

const MIN_RIVETKIT_VERSION_SCHEDULES = "2.3.4";
const MIN_RIVETKIT_VERSION_INSPECTOR_NEGOTIATION = "2.3.4";

v2.3.4 was already published on 2026-07-15 (gh api repos/rivet-dev/rivet/releases confirms the tag), four days before this PR's scheduling feature exists. Any actor already running published v2.3.4 (which has no cron/interval scheduling support at all) will be reported by isVersionAtLeast(version, MIN_RIVETKIT_VERSION_SCHEDULES) as schedule-capable, and the dashboard will show/query the new Schedules tab against a runtime that doesn't implement it. Compare with the existing pattern for other gates in the same file (MIN_RIVETKIT_VERSION_QUEUE = "2.0.40", MIN_RIVETKIT_VERSION_TABCONFIG_INIT = "2.3.3"), which were each set to the version that actually shipped the corresponding feature. This constant needs to point at whatever version will actually ship scheduling (the next unreleased version after 2.3.4), not the one that already shipped without it.

Previously-flagged schema migration issue looks resolved

An earlier pass on this PR flagged internal_schema.rs editing MIGRATIONS[1] in place (folding new schedule columns/table into the existing schema-v2 migration group) as a high-severity break for actors already persisted at schema v7. That's since been addressed: the file now carries an explicit invariant comment ("safe only while no internal schema version has shipped; after release, all changes must be appended as new migrations"), and a new test (unpublished_schema_has_explicit_values_and_minimal_constraints) enforces no-DEFAULT/minimal-constraint shape to keep the ladder rewritable. I independently confirmed INTERNAL_SCHEMA_VERSION = 7 has never shipped: the SQLite internal-schema migration system was only merged into main on 2026-07-18 (#5384), and the last actual release (v2.3.4) predates that by three days. So the in-place edit is safe as documented, no longer a blocker.

Other findings

Frontend

  • Bug: frontend/src/components/actors/actor-schedules-format.ts formatDuration() never special-cases a magnitude of 1, so durations render as "Every 1 hours", "1 seconds", "1 minutes", "1 days". actor-schedules-tab.test.ts only exercises 5 minutes, 1.5 minutes, 2 hours, 2 days, so the singular case isn't covered and shipped uncaught.
  • Test coverage: despite its name, actor-schedules-tab.test.ts only tests the formatting helpers, not ActorSchedulesTab/ScheduleDetails themselves (no coverage of empty state, history status states, or the delete-confirmation flow).
  • No feature-flag (frontend/src/lib/features.ts) gating on the new tab. It's gated only by the runtime-version capability check above. That may be intentional since scheduling is core actor behavior rather than flavor-specific, but worth a quick confirmation given CLAUDE.md's guidance to flag new whole-tab subsystems.
  • Forms, discriminated-union exhaustiveness (.exhaustive() on the new schedule tags), and error shapes all check out.

rivetkit-core (schedule.rs)

  • The global _rivet_schedule_history prune (DELETE ... WHERE id NOT IN (SELECT id ... ORDER BY fired_at DESC, id DESC LIMIT ?)) runs on every recurring fire but only _rivet_schedule_history_schedule (schedule_id, fired_at DESC) is indexed. No index on fired_at alone, so this is a full-table scan + sort up to MAX_ACTOR_HISTORY rows every fire (as often as every 5s per schedule). Bounded, but worth an index or a less frequent prune cadence.
  • read_stored_schedule's per-kind validation match is guard-based (ScheduleKind::At if ..., etc.) falling through to a bare _ => Ok(event), rather than exhaustively matching event.kind first. A future 4th ScheduleKind variant would silently skip validation instead of failing to compile, which is the exact failure mode the project's "no _ => on enum matches" rule targets.
  • DST handling, interval catch-up math, overlap/skip semantics, and the schedule-capacity/mutation locking are all well tested (tests/schedule.rs covers DST fold/gap, min interval, max history, interrupted recovery, concurrent distinct-name creates). No issues found there.

Inspector protocol (v6.bare / versioned.rs / inspector_ws.rs)

  • v6 is purely additive over v5; both Rust (versioned.rs) and TS (client.browser.ts) converters are hand-written field-by-field, no serialize/deserialize round-trip shortcuts. Older clients correctly receive a structured inspector.schedules_dropped error instead of a broken payload.
  • Minor: the new protocol-version-mismatch rejection uses close code 1002, while the existing auth rejection in the same handler uses 1008 per the inspector.unauthorized convention. Not wrong, but introduces a second close-code convention in one file, worth confirming intentional.
  • Schedule.maxHistory on the wire reuses a timestamp_to_uint() helper for what is actually a plain bounded count, not a timestamp. Harmless but misleading naming.

NAPI / WASM / rivetkit (Rust)

  • Layering is clean: rivetkit-napi/src/schedule.rs and rivetkit-wasm/src/lib.rs are pure delegates to rivetkit-core::actor::schedule with no cron/history logic duplicated. napi-runtime.ts/wasm-runtime.ts stay fully symmetric for the new schedule surface.
  • Gap: rivetkit-rust/packages/rivetkit/src/start.rs destructures ActionCall { name, args, conn, reply, .. }, still silently dropping scheduled_fire (the .. was added specifically to ignore the new field). Rust SDK actor handlers have no way to tell a scheduled fire from a regular action call, unlike the TypeScript SDK's ActionCall::scheduled_fire() equivalent. Per this repo's "mirror new TS capabilities to Rust where practical" rule, this is a tracked-worthy parity gap rather than a blocker.
  • Test gap: tests/runtime-parity.test.ts runs the new scheduled-fire-metadata test only for "napi", breaking the file's otherwise-consistent test.each(["napi", "wasm"]) pattern. The WASM scheduled-fire injection path has no TS-level test coverage. cron-api.test.ts similarly only exercises the native/NAPI registry adapters, not WasmCoreRuntime.

Summary

Design (versioned inspector protocol bump, bounded history, core-owns-lifecycle layering, DST-aware cron scheduling) is solid and generally well tested. The version-gate constant reusing an already-shipped release number is a real correctness bug worth fixing before merge; the duration-formatting singular/plural bug is a small but user-visible cosmetic fix. Everything else here is coverage/consistency polish, not a blocker.

@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5427 July 19, 2026 14:56 Destroyed
@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5427 July 19, 2026 15:24 Destroyed
@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5427 July 19, 2026 15:33 Destroyed
@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5427 July 19, 2026 15:35 Destroyed
@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5427 July 19, 2026 15:38 Destroyed
@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5427 July 19, 2026 15:40 Destroyed
@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5427 July 19, 2026 15:41 Destroyed
@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5427 July 19, 2026 15:42 Destroyed
@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5427 July 19, 2026 15:43 Destroyed
@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5427 July 19, 2026 15:43 Destroyed
@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5427 July 19, 2026 15:45 Destroyed
@claude

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Review

Summary

This fixes a real bug. cron_delete previously only removed the row from _rivet_schedule_events, leaving _rivet_schedule_history rows orphaned under the same schedule_id (cron:<name>). Since cron names are reused after delete, a freshly created cron with the same name would inherit the old cron's history. The fix wraps the history delete and event delete in one execute_batch transaction, gated by an EXISTS check against the still-un-deleted event row so it only fires for real recurring (non-At) schedules. The added test deleting_recurring_schedule_removes_history_before_name_reuse directly exercises the previously broken path. Correct fix, well tested.

The global history-cap prune query was also rewritten from a NOT IN (subquery LIMIT ?) anti-join, which requires materializing up to MAX_ACTOR_HISTORY (10,000) kept ids and scanning the whole table against them on every schedule fire, to a direct id IN (SELECT id ... ORDER BY fired_at ASC, id ASC LIMIT MAX(COUNT(*) - ?, 0)), which only selects and deletes the actual overflow rows. The new actor_history_is_globally_bounded test asserts MIN(fired_at)/MAX(fired_at) after pruning, not just the row count, so it correctly verifies the oldest rows were the ones removed rather than an arbitrary set that happens to leave the count right. Good, meaningful test strengthening.

Findings

  1. (high) Schema migration is edited in place instead of appended, so already-provisioned actor databases will never pick up the new or changed indexes.

internal_schema.rs modifies MIGRATIONS[1] directly, adding id DESC to _rivet_schedule_history_schedule and adding the new _rivet_schedule_history_fired_at index, without bumping INTERNAL_SCHEMA_VERSION (still 6). ensure_internal_schema only runs MIGRATIONS[current_version..INTERNAL_SCHEMA_VERSION]. For any actor SQLite database that already recorded schema_version = 6, current_version == INTERNAL_SCHEMA_VERSION short-circuits and the migration ladder never re-runs, so those actors keep the old _rivet_schedule_history_schedule (schedule_id, fired_at DESC) index and never get _rivet_schedule_history_fired_at at all, the index the new global-prune query in this PR depends on for its efficiency win. New actors get the corrected schema fine since the whole ladder runs in one shot for them.

The comment at the top of MIGRATIONS states this exact rule: rewriting entries in place is safe only while no internal schema version has shipped, and after release all changes must be appended as new migrations with INTERNAL_SCHEMA_VERSION advanced. _rivet_schedule_events/_rivet_schedule_history (schema v2) landed in #5384 (merged 2026-07-18), one day before this PR, and no tagged release since then includes it (last release v2.3.4 predates #5384), so this is likely still safe in practice today. But it is exactly the case the file's own invariant warns about: if any actor database was already provisioned off main between #5384 and this PR (dev/staging, self-hosted builds off main, etc.), this change silently never reaches it.

Recommend appending a new migration group instead (CREATE INDEX IF NOT EXISTS _rivet_schedule_history_fired_at ..., plus a drop/recreate to add id DESC to _rivet_schedule_history_schedule) and bumping INTERNAL_SCHEMA_VERSION to 7, per the documented convention, rather than relying on MIGRATIONS[1] never having shipped anywhere.

  1. (medium) cron_delete_if_action, the sibling auto-cleanup path, still leaves history orphaned, the same bug this PR fixes for cron_delete.

context.rs calls ctx.cron_delete_if_action(&name, &action_name) (schedule.rs:451) when a recurring schedule fires against an action that no longer exists, to self-remove the dangling cron. That method only deletes the _rivet_schedule_events row; it was not updated alongside cron_delete to also clear _rivet_schedule_history. The existing test missing_recurring_action_records_error_and_deletes_job (tests/schedule.rs:316) explicitly asserts cron_history("missing", None) still has one entry after the auto-delete, so this is documented, current behavior, not an accident of this diff. But it means the exact scenario this PR sets out to fix (stale history resurfacing when a cron name is reused) still reproduces through this path: an action gets removed, the cron auto-deletes with its error history intact, the action is later restored and cron_every/cron_set is called again for the same name, and the new cron's history now starts out mixed with the old error entries. Worth deciding intentionally whether history should survive an auto-delete (arguably useful for post-mortem) and if so, scoping/tagging those rows so they don't bleed into a later reincarnation of the same name, rather than leaving the two delete paths inconsistent.

  1. (minor) The global-cap prune still does a full COUNT(*) scan on every history insert.

history_prune_statements's second statement runs SELECT COUNT(*) FROM _rivet_schedule_history (unscoped) on every recurring-schedule fire and every cron_set/cron_every call. With the new _rivet_schedule_history_fired_at index this is index-only and cheap relative to the old NOT IN full-table antijoin, but it's still O(total history rows) per call rather than O(1). Worth confirming this cost is acceptable near the 10,000-row ceiling for actors with several frequently-firing schedules feeding the shared history table. Not a blocker, just flagging in case "efficiently" in the title implies a stronger bound was intended (e.g. a maintained counter).

Nits

  • The EXISTS guard in cron_delete and the removed computation, keyed off results.get(1) (the events-delete result, not the history-delete result), correctly preserve the prior return-value semantics, but the index-into-results coupling to statement order is easy to break silently on a future edit. A one-line comment would help.

@NathanFlurry NathanFlurry changed the title feat(rivetkit): add actor scheduling fix(rivetkit): clean up cron history efficiently Jul 19, 2026
@NathanFlurry
NathanFlurry changed the base branch from main to stack/docs-rivetkit-remove-redundant-overdue-schedule-note-pkwwuxmn July 19, 2026 22:24
@NathanFlurry
NathanFlurry force-pushed the stack/docs-rivetkit-remove-redundant-overdue-schedule-note-pkwwuxmn branch from 05a8348 to 5112829 Compare July 20, 2026 06:46
@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5427 July 20, 2026 06:46 Destroyed
@NathanFlurry
NathanFlurry changed the base branch from stack/docs-rivetkit-remove-redundant-overdue-schedule-note-pkwwuxmn to main July 20, 2026 08:37
@NathanFlurry
NathanFlurry merged commit 320bb21 into main Jul 20, 2026
13 of 18 checks passed
@NathanFlurry
NathanFlurry deleted the cron branch July 20, 2026 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant