Skip to content

refactor(rivetkit-core): remove prevent_sleep field and deprecate set_prevent_sleep to no-op#4741

Draft
NathanFlurry wants to merge 1 commit intodb-fuzzer-kitchen-sinkfrom
sleep-cleanup/01-core-remove-prevent-sleep
Draft

refactor(rivetkit-core): remove prevent_sleep field and deprecate set_prevent_sleep to no-op#4741
NathanFlurry wants to merge 1 commit intodb-fuzzer-kitchen-sinkfrom
sleep-cleanup/01-core-remove-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 #4741 Review: refactor(rivetkit-core): remove prevent_sleep field and deprecate set_prevent_sleep to no-op

Overview

This PR removes the prevent_sleep field from rivetkit-core's ActorContextInner and converts set_prevent_sleep / prevent_sleep into deprecated no-ops, while retaining the NAPI surface for bridge compatibility. The intent is to replace the old pull-style flag with keep_awake (RAII guard) and wait_until (future-scoped hold).


Critical: AgentOS regression

The agentOS layer (agent-os/actor/index.ts, process.ts, shell.ts, session.ts) still calls syncPreventSleep(c)c.setPreventSleep(shouldPrevent) to hold the actor awake while a subprocess or shell session is running. After this change, those calls go through the NAPI bridge into a no-op. AgentOS actors running on the native (NAPI) path can now sleep prematurely while a process is still alive. This is a behavioral regression that needs to be fixed before or alongside this change.

Tests will pass vacuously or fail

actor-sleep.test.ts lines 596–691 test three concrete scenarios: that preventSleep blocks auto-sleep until cleared, delays shutdown, and can be restored on wake. All three rely on setPreventSleep actually preventing sleep. Against the NAPI driver these tests now exercise a no-op and either:

  • pass vacuously (the actor was already sleeping for another reason), or
  • fail because the expected sleep-blocking behavior is gone.

These tests should be removed, skipped for the native driver, or replaced with equivalent keep_awake / wait_until tests before this ships.

TypeScript pure path and NAPI path now diverge

The pure-TypeScript actor instance (actor/instance/mod.ts) retains the full preventSleep state machine — #preventSleep, #waitForPreventSleepClear, CanSleep.PreventSleep, etc. The NAPI path converts those calls to no-ops. Callers currently cannot tell which runtime they're on, so the same actor code exhibits different sleep behavior depending on the driver. The PR should either:

  • stub out the TS side too (marking it deprecated and always returning false), or
  • document the intentional split and note that the TS path will be updated in a follow-up.

Minor: #[allow(deprecated)] without an explanatory comment

The #[allow(deprecated)] suppression in rivetkit-napi/src/actor_context.rs is correct, but a one-line comment explaining why ("NAPI bridge retains this surface for TS callers still using setPreventSleep") would make the intent clearer to future readers.

Positive notes

  • The removal of prevent_sleep_notify and the PreventSleep variant from CanSleep is clean; no dead branches left.
  • Propagating #[deprecated] through both rivetkit-core and the thin rivetkit (Rust) wrapper with #[allow(deprecated)] in the forwarders is the correct pattern.
  • Keeping the NAPI surface as a no-op rather than removing it avoids a hard break in downstream TS code that may still call setPreventSleep.

Summary

The core cleanup is well-motivated, but the PR has a behavioral regression in agentOS and leaves tests that now exercise a no-op. The asymmetry between the TS-pure and NAPI paths also needs to be called out explicitly (in the PR description at minimum). Recommend resolving the agentOS syncPreventSleep calls and the affected test suite before marking ready for review.

@NathanFlurry NathanFlurry force-pushed the sleep-cleanup/01-core-remove-prevent-sleep branch from c43b558 to f4dbc84 Compare April 24, 2026 11:48
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