Skip to content

fix(rivetkit): run onSleep after in-flight work and refuse dispatch once it starts - #5860

Open
eersnington wants to merge 1 commit into
mainfrom
stack/fix-rivetkit-run-onsleep-after-in-flight-work-kttnpoyx
Open

eersnington wants to merge 1 commit into
mainfrom
stack/fix-rivetkit-run-onsleep-after-in-flight-work-kttnpoyx

Conversation

@eersnington

@eersnington eersnington commented Oct 11, 2026 •

Copy link
Copy Markdown
Member

Run onSleep after in-flight work finishes, and refuse new work once onSleep starts. Before this change, an action running when sleep started could fail because TypeScript closed SQLite right after onSleep.

Before

sequenceDiagram
	participant E as Engine
	participant T as ActorTask (core)
	participant TS as TypeScript runtime
	participant A as In-flight action
	E->>T: Stop(Sleep)
	T->>TS: onSleep
	TS->>TS: close SQLite, clear vars
	A--xTS: c.db.execute() fails
	E->>T: new action (still accepted)
	T->>T: sync_alarm fails (SQLite already closed)
Loading

After

sequenceDiagram
	participant E as Engine
	participant T as ActorTask (core)
	participant TS as TypeScript runtime
	participant A as In-flight action
	E->>T: Stop(Sleep)
	T->>T: fire abort signal
	A->>TS: c.db.execute() completes
	Note over T: wait until in-flight work drains, or 3/4 of the grace period
	T->>TS: onSleep + disconnects
	E->>T: new action
	T-->>E: actor.restarting (503), client retries on the next generation
	T->>T: sync_alarm, then close SQLite
Loading

Changes

  • Order: ActorTask runs the sleep hooks when the shutdown counter (actions, keepAwake, waitUntil, registered tasks) and WebSocket callbacks reach zero, or at the hooks deadline. Raw onRequest and run are not waited for. Destroy runs its hooks at once, as before.
  • Reserve: SLEEP_HOOK_RESERVE_DIVISOR = 4 keeps the last quarter of the grace period for onSleep, disconnects, and the final save. With the 15 second default this is 3.75 seconds.
  • New error: actor.restarting (503), "Actor is restarting. Retry the request." Dispatch is refused with it from the moment the hooks run until finalize. Before the hooks run, dispatch is still accepted, because the client retry budget for this error is short. DestroyGrace keeps actor.stopping.
  • SQLite close moves to core: On the drained path, cleanupNativeSleepRuntimeState no longer closes SQLite. Core runs sync_alarm and then closes it, so the alarm sync no longer fails on every sleep. If work did not drain by the deadline, TypeScript still closes SQLite early so late work cannot write after shutdown.
  • try_finish_grace requires the hooks to have run, so an idle actor cannot finalize without onSleep.
  • docs-internal/engine/sleep-sequence.md describes the new order.

Behavior change

An action that waits for something onSleep does, such as a flag that onSleep sets, now waits until the hooks deadline. Long actions should watch c.abortSignal, which still fires when sleep starts. No onSleep in the docs or examples uses this pattern (going to follow up on this)

@railway-app

railway-app Bot commented Oct 11, 2026

Copy link
Copy Markdown

This PR was not deployed automatically as @eersnington does not have access to the Railway project.

In order to get automatic PR deploys, please add @eersnington to your workspace on Railway.

@claude

claude Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Review: run onSleep after in-flight work, refuse dispatch once it starts

The design is sound. Moving the SQLite close into core after sync_alarm fixes a real ordering bug. The paused-time Rust tests assert ordering deterministically. A few concerns:

Likely bug: the client does not retry actor.restarting
The PR says clients retry on the next generation. The TS client only recognizes code === "stopping" for lifecycle retry: client/lifecycle-errors.ts:61,117, client/actor-query.ts:54, and several sites in client/actor-handle.ts (250, 472, 534, 803, 878). This PR does not touch them. Unless a generic 503 path covers it, restarting will surface as a hard error instead of a retry. Add restarting next to stopping in those places and add a client test. I'd fix this before merge.

Missing generated artifacts
CLAUDE.md requires pnpm docs:gen-api after adding a RivetError, and committing docs/api/error-registry.json. That file is not in the diff, so pnpm docs:check-api will likely fail in CI. Also, actor.stopping.json and actor.destroying.json live in engine/artifacts/errors/, while the new file is in rivetkit-rust/engine/artifacts/errors/. Both directories exist, so please confirm which one cargo check writes to.

Behavior and edge cases

  • SleepFinalize and post-hooks SleepGrace now return restarting instead of stopping. Anything matching stopping for sleep changes behavior, so grep the docs and clients for it.
  • deadline - grace_period / 4 is Instant - Duration, which can panic on underflow. Use checked_sub, or compute now + grace_period - grace_period / 4.
  • Both the hooks-deadline arm and the grace-deadline arm can be ready at once for tiny grace periods. That is harmless because emit_grace_hooks is idempotent, but a short comment would help.
  • Consider logging a warning when the hooks deadline fires with tracked work still running, so users can diagnose the documented "action waits on onSleep" behavior change.
  • When work does not drain, TS closes SQLite early and core closes it again later. Please confirm the double close is idempotent.

Tests

  • In tests/task.rs, the new while ctx.shutdown_task_count() != 0 { yield_now().await } loop is a poll loop. Prefer a notify-based wait such as AsyncCounter::wait_zero, per CLAUDE.md.
  • There is no coverage of the not-drained path, where work outlives the hooks deadline and TS closes SQLite early. Please add a driver or Rust test for it.

Nits

  • GraceHooks::Pending { deadline } could be a tuple variant.
  • sleep-sequence.md writes actor/restarting, while the code and artifacts use actor.restarting. Use one form.

🤖 Generated with Claude Code

This branch has not been deployed

No deployments
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