Skip to content

fix(rivetkit): persist native onSleep state on errors#4936

Merged
NathanFlurry merged 1 commit intomainfrom
05-04-fix_rivetkit_persist_onsleep_error_state
May 5, 2026
Merged

fix(rivetkit): persist native onSleep state on errors#4936
NathanFlurry merged 1 commit intomainfrom
05-04-fix_rivetkit_persist_onsleep_error_state

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Copy Markdown
Member Author

NathanFlurry commented May 4, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 4, 2026

Code Review: fix(rivetkit): persist native onSleep state on errors

Overview

This PR fixes a bug where actor state was not persisted when the onSleep callback threw an error. The fix wraps onSleep(actorCtx) in a try/finally block so the state save runs unconditionally, even on error.


Correctness

The fix is sound. Previously, a thrown exception in onSleep would skip the save path and fall through to the outer finally (database close, state clear, dispose), leaving state un-persisted before sleep. The try/finally restructure is the correct pattern here.

One subtle risk: if onSleep throws and the subsequent saveState/actorSaveState in the finally block also throws, the original onSleep error is silently discarded. This is inherent to JS try/finally semantics and is likely acceptable (a save failure is more actionable), but worth noting.


Consistency with Other Lifecycle Hooks

  • onDestroy does not save state (correct, since a destroyed actor needs no persistence).
  • onBeforeActorStart disposes in a finally but does not save state.
  • onSleep is the only lifecycle hook where a post-callback state persist is meaningful, so the asymmetry is intentional.

Code Quality

Minimal and well-scoped. The existing outer finally block for cleanup (closeDatabase, clearNativeRuntimeState, dispose) is left intact, and the inner try/finally nests cleanly within it.


Test Coverage

No test verifies that state persists when onSleep throws. A driver test would strengthen this fix: have onSleep throw after mutating state, then verify the state is correct when the actor re-wakes.


Summary

Category Assessment
Correctness Correct fix for the described bug
Risk Low - minimal, well-contained change
Test coverage Missing; a regression test is recommended
Style/conventions Consistent with surrounding code

The fix is correct. Recommend adding a test to guard the error-path behavior.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Preview packages published to npm

Install with:

npm install rivetkit@pr-4936

All packages published as 0.0.0-pr.4936.fa84f39 with tag pr-4936.

Engine binary is shipped via @rivetkit/engine-cli on linux-x64-musl, linux-arm64-musl, darwin-x64, and darwin-arm64. Windows users should use the release installer or set RIVET_ENGINE_BINARY.

Docker images:

docker pull rivetdev/engine:slim-fa84f39
docker pull rivetdev/engine:full-fa84f39
Individual packages
npm install rivetkit@pr-4936
npm install @rivetkit/react@pr-4936
npm install @rivetkit/rivetkit-napi@pr-4936
npm install @rivetkit/workflow-engine@pr-4936

@NathanFlurry NathanFlurry force-pushed the 05-04-fix_rivetkit_sleep_grace_dispatch branch from 5ff98a4 to e054f95 Compare May 4, 2026 09:10
@NathanFlurry NathanFlurry force-pushed the 05-04-fix_rivetkit_persist_onsleep_error_state branch from 28a6369 to b40eb33 Compare May 4, 2026 09:10
Base automatically changed from 05-04-fix_rivetkit_sleep_grace_dispatch to main May 5, 2026 14:57
@NathanFlurry NathanFlurry merged commit b40eb33 into main May 5, 2026
42 of 54 checks passed
@NathanFlurry NathanFlurry deleted the 05-04-fix_rivetkit_persist_onsleep_error_state branch May 5, 2026 14:58
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