Skip to content

refactor(rivetkit): deprecate setPreventSleep to no-op in TypeScript#4747

Draft
NathanFlurry wants to merge 1 commit intosleep-cleanup/05-grace-deadline-counter-logfrom
sleep-cleanup/06-ts-deprecate-prevent-sleep
Draft

refactor(rivetkit): deprecate setPreventSleep to no-op in TypeScript#4747
NathanFlurry wants to merge 1 commit intosleep-cleanup/05-grace-deadline-counter-logfrom
sleep-cleanup/06-ts-deprecate-prevent-sleep

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 Apr 24, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

PR Review: refactor(rivetkit): deprecate setPreventSleep to no-op in TypeScript

Overview

This PR deprecates the boolean setPreventSleep / preventSleep API in favor of the promise-based keepAwake(promise) and waitUntil(promise) pattern. The native adapter and workflow context both stub out the old API as no-ops. The approach is sound — promise-based lifecycle tracking is cleaner and avoids the misuse patterns that the boolean flag enabled.


Issues

Behavioral change with no-op preventSleep getter (minor risk)

The getter preventSleep now always returns false instead of the actual tracked value. Any consumer that reads this value to make a decision (e.g., if (ctx.preventSleep) { ... }) will now silently see false even if sleep was logically blocked. This is a correct deprecation but is a subtle semantic break rather than a pure no-op. Worth calling out explicitly in the deprecation notice or changelog.

workflow/context.ts setter still calls #ensureActorAccess (minor inconsistency)

setPreventSleep(_prevent: boolean): void {
    this.#ensureActorAccess("setPreventSleep");  // still throws if called outside valid context
}

The setter is described as a no-op, but it still throws if called from an invalid context. This is inconsistent with the native.ts adapter where the stub is a pure no-op with no access check. The behavior should be consistent or the inconsistency documented.

TODO comment wording (instance/mod.ts)

// TODO(sleep-cleanup): investigate whether ActorInstance / drivers/engine
// path is still needed after native path maturation. preventSleep tracking
// below is retained for legacy driver compatibility but no longer drives core.

Per CLAUDE.md conventions, comments should not document deltas ("retained for legacy driver compatibility") since that context rots quickly. Consider trimming to just "preventSleep tracking is a legacy driver compatibility shim" and moving the investigation note to .agent/todo/.

No tests added

The PR checklist items for tests are unchecked. Since preventSleep now always returns false and setPreventSleep is a no-op, a regression could silently reintroduce the native call. A simple test asserting ctx.preventSleep === false and that setPreventSleep does not throw would provide a safety net.

Removal version consistency

Both @deprecated notices say "Will be removed in 2.2.0". Confirm this matches the actual release plan — if the version target changes, both spots will need updating.


Suggestions

  • Consider checking whether setPreventSleep appears anywhere in docs/guides and updating those callsites.
  • The keepAwake JSDoc update (removing the old @deprecated pointing back to setPreventSleep) is correct and well-done.

Summary

The direction is correct — promise-scoped lifecycle tracking is a better API. The main risk is the preventSleep getter silently returning false for any code that was reading it. The no-test concern and TODO comment wording are minor. The #ensureActorAccess inconsistency between workflow/context.ts and native.ts is worth resolving for consistency.

@NathanFlurry NathanFlurry force-pushed the sleep-cleanup/05-grace-deadline-counter-log branch from b1d5d9d to 70609c2 Compare April 24, 2026 13:16
@NathanFlurry NathanFlurry force-pushed the sleep-cleanup/06-ts-deprecate-prevent-sleep branch from dfa956c to 181bb0f Compare April 24, 2026 13:16
@github-actions
Copy link
Copy Markdown
Contributor

Preview packages published to npm

Install with:

npm install rivetkit@pr-4747

All packages published as 0.0.0-pr.4747.3298821 with tag pr-4747.

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-3298821
docker pull rivetdev/engine:full-3298821
Individual packages
npm install rivetkit@pr-4747
npm install @rivetkit/react@pr-4747
npm install @rivetkit/rivetkit-napi@pr-4747
npm install @rivetkit/workflow-engine@pr-4747

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