feat(core): actor sleeping #1178
Conversation
|
Claude finished @NathanFlurry's task —— View job Code Review: Actor Sleeping FeatureTodo List:
OverviewThis PR introduces an actor sleeping mechanism that allows inactive actors to be put into a dormant state to conserve resources. The feature includes:
Code Quality & Best Practices ✅Strengths:
Minor improvements needed:
Architecture & Design 🔧Good design decisions:
Potential concerns:
Potential Bugs & Issues
|
|
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.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
packages/core/src/actor/instance.ts
Outdated
| if (this.#config.options.noSleep || !this.#sleepingSupported) return; | ||
|
|
||
| if (this.#sleepTimeout) { | ||
| clearInterval(this.#sleepTimeout); |
There was a problem hiding this comment.
The code is using clearInterval() to clear a timeout that was set with setTimeout(). For proper timer management, clearTimeout() should be used instead to match the timer creation method.
| clearInterval(this.#sleepTimeout); | |
| clearTimeout(this.#sleepTimeout); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
graphite-app
bot
changed the base branch from
08-13-feat_engine_add_engine_driver
to
graphite-base/1178
NathanFlurry
force-pushed
the
08-17-feat_core_actor_sleeping
branch
from
97e285f to
b0d246b
Compare
NathanFlurry
force-pushed
the
graphite-base/1178
branch
from
5a74e24 to
fa0d38d
Compare
|
Claude finished @NathanFlurry's task —— View job Code Review: Actor Sleeping FeatureThis PR introduces an actor sleeping mechanism that allows inactive actors to be put into a dormant state to conserve resources. Here's my comprehensive analysis: OverviewWhat this adds:
🐛 Critical Issues Found1. Timer Management Bug (packages/core/src/actor/instance.ts:1454)clearInterval(this.#sleepTimeout); // ❌ Wrong - clearing interval for timeoutIssue: Using Fix: Should use clearTimeout(this.#sleepTimeout); // ✅ Correct2. Engine Driver Error Handling (packages/core/src/drivers/engine/actor-driver.ts:357-359)async sleep(actorId: string) {
this.#runner.sleepActor(actorId); // ❌ Missing await and error handling
}Issue: The 3. Inconsistent Sleep Implementation
🏗️ Architecture & Design✅ Well-designed aspects:
🔒 Security Analysis✅ No security concerns identified:
🚀 Performance Implications✅ Positive impacts:
Minimal overhead: Timer management adds small constant overhead but connection tracking reuses existing logic efficiently. 📋 Code Quality & Best Practices✅ Strengths:
🧪 Test CoverageTest modifications reviewed:
Recommendation: Add integration tests for:
📝 Recommended ChangesHigh Priority
Medium Priority
✅ Final AssessmentThis is a well-implemented feature that adds valuable resource management capabilities to RivetKit. The code follows established patterns, maintains backward compatibility, and includes proper safeguards. Recommendation: ✅ Approve with required fixes The timer management bug should be fixed before merge, but overall this is solid work that will improve the platform's resource efficiency. |
|
Claude finished @NathanFlurry's task —— View job Code Review: Actor Sleeping Feature Todo List:
OverviewThis PR introduces an actor sleeping mechanism that allows inactive actors to be put into a dormant state to conserve resources. The feature includes:
Code Quality & Best Practices ✅Strengths:
Issues to address:
Critical Bug Found 🚨Timer Management Issue (identified by Graphite review bot): // packages/core/src/actor/instance.ts:1454
clearInterval(this.#sleepTimeout);Problem: The code uses Fix needed: Should use clearTimeout(this.#sleepTimeout);Architecture & Design 🔧Good design decisions:
Potential concerns:
Additional Issues Found
|
NathanFlurry
force-pushed
the
08-17-feat_core_actor_sleeping
branch
from
b0d246b to
7700f30
Compare
NathanFlurry
changed the base branch from
graphite-base/1178
to
09-08-fix_core_revert_deleted_fixtures
|
Claude finished @NathanFlurry's task —— View job Code Review: Actor Sleeping Feature Todo List:
OverviewThis PR introduces an actor sleeping mechanism that allows inactive actors to be put into a dormant state to conserve resources. The feature includes:
🚨 Critical Bug FoundTimer Management Issue (packages/core/src/actor/instance.ts:1454): clearInterval(this.#sleepTimeout); // ❌ Wrong - clearing interval for timeoutProblem: The code uses Fix needed: Should use clearTimeout(this.#sleepTimeout); // ✅ CorrectThis matches the issue identified by the Graphite review bot and must be fixed before merge. 🏗️ Architecture & Design✅ Well-designed aspects:
🐛 Additional Issues Found
🔒 Security Analysis✅ No security concerns identified:
🚀 Performance Implications✅ Positive impacts:
Minimal overhead: Timer management adds small constant overhead but connection tracking reuses existing logic efficiently. 📋 Code Quality & Best Practices✅ Strengths:
🧪 Test CoverageTest modifications reviewed:
Recommendation: Add integration tests for:
📝 Required ChangesHigh Priority (Must Fix Before Merge)
Medium Priority (Recommended Improvements)
✅ Final AssessmentThis is a well-designed feature that adds valuable resource management capabilities to RivetKit. The code follows established patterns, maintains backward compatibility, and includes proper safeguards. Recommendation: The sleep functionality will significantly improve resource efficiency by allowing inactive actors to be suspended, which is especially valuable for platforms handling many actors with varying activity levels. |
|
Claude finished @NathanFlurry's task —— View job Code Review: Actor Sleeping Feature Todo List:
OverviewThis PR introduces an actor sleeping mechanism that allows inactive actors to be put into a dormant state to conserve resources. The feature includes:
🚨 CRITICAL BUG - MUST FIXTimer Management Issue (packages/core/src/actor/instance.ts:1454)clearInterval(this.#sleepTimeout); // ❌ WRONGProblem: Using Required Fix: clearTimeout(this.#sleepTimeout); // ✅ CORRECT
|
NathanFlurry
force-pushed
the
08-17-feat_core_actor_sleeping
branch
from
7700f30 to
867931a
Compare
|
Claude finished @NathanFlurry's task —— View job Code Review: Actor Sleeping Feature Todo List:
OverviewThis PR introduces an actor sleeping mechanism that allows inactive actors to be put into a dormant state to conserve resources. The feature includes:
✅ Major Issues ResolvedTimer Management Bug FixedThe timer management issue flagged by Graphite has been resolved. Line 1454 in 🏗️ Architecture & Design✅ Well-designed aspects:
Driver ImplementationsEngine Driver (packages/core/src/drivers/engine/actor-driver.ts:352-354):
File System Driver (packages/core/src/drivers/file-system/actor.ts:85-87):
🐛 Issues FoundMinor Issues
🔒 Security Analysis✅ No security concerns identified:
🚀 Performance Implications✅ Positive impacts:
Performance considerations:
📋 Code Quality & Best Practices✅ Strengths:
|
NathanFlurry
force-pushed
the
08-17-feat_core_actor_sleeping
branch
from
867931a to
620dda3
Compare
|
Claude finished @NathanFlurry's task —— View job Code Review: Actor Sleeping Feature Todo List:
OverviewThis PR introduces an actor sleeping mechanism that allows inactive actors to be put into a dormant state to conserve resources. The feature includes:
✅ Critical Issue ResolvedTimer Management Bug FixedThe timer management issue flagged by previous reviews has been resolved. Line 1492 in 🏗️ Architecture & Design✅ Excellent Design Decisions
Driver ImplementationsEngine Driver (packages/core/src/drivers/engine/actor-driver.ts:352-354):
File System Driver (packages/core/src/drivers/file-system/actor.ts:85-87):
🧪 Test Coverage - Outstanding✅ Comprehensive Test Suite AddedThe PR includes excellent test coverage with: New Test Fixtures (
Integration Tests (
Test Fixture Improvements:
🔒 Security Analysis✅ No security concerns identified:
🚀 Performance Implications✅ Excellent Performance Benefits
Performance Considerations
📋 Code Quality & Best Practices✅ Outstanding Implementation Quality
Sleep Logic ImplementationThe
|
| while (entry.queuedWrite) { | ||
| // Remove pending write resolver so we can write a new write resolver for the next queued operation | ||
| // | ||
| // If we read entry.pendingWriteResolver directly, we would be resolving *all* waiting promises, not just the ones for this specific queued write | ||
| const pendingWriteResolver = entry.pendingWriteResolver; | ||
| entry.pendingWriteResolver = undefined; | ||
|
|
||
| const writeOperation = entry.queuedWrite; | ||
| entry.queuedWrite = undefined; | ||
| try { | ||
| await writeOperation(); | ||
| } catch (err) { | ||
| logger().error("failed to write state", { err: stringifyError(err) }); | ||
| } | ||
|
|
||
| // Resolve all currently waiting writes after this operation completes | ||
| pendingWriteResolver?.resolve(); | ||
| } |
There was a problem hiding this comment.
There appears to be a potential race condition in the write queue implementation. The current code sets entry.pendingWriteResolver = undefined before awaiting the write operation. If another writeActor() call occurs during this window, it will create a new resolver that won't be resolved by the current operation.
Consider restructuring to ensure all pending resolvers are properly handled:
// Store resolver before clearing it
const pendingWriteResolver = entry.pendingWriteResolver;
entry.pendingWriteResolver = undefined;
// Execute write operation
try {
await writeOperation();
} catch (err) {
logger().error("failed to write state", { err: stringifyError(err) });
}
// Resolve after operation completes
pendingWriteResolver?.resolve();This pattern ensures that any new write requests that come in during the operation will create a new resolver that will be handled by the next iteration of the loop.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| async _sleep() { | ||
| invariant(this.#sleepingSupported, "sleeping not supported"); | ||
| invariant(this.#actorDriver.sleep, "no sleep on driver"); | ||
|
|
||
| if (this.#sleepCalled) { | ||
| logger().warn("already sleeping actor"); | ||
| return; | ||
| } | ||
| this.#sleepCalled = true; | ||
|
|
||
| logger().info("actor sleeping"); | ||
|
|
||
| // The actor driver should call stop when ready to stop | ||
| // | ||
| // This will call _stop once Pegboard responds with the new status | ||
| this.#actorDriver.sleep(this.#actorId); | ||
| } |
There was a problem hiding this comment.
Consider adding error handling to the _sleep() method. Currently, if this.#actorDriver.sleep(this.#actorId) throws an exception, the actor will remain in an inconsistent state with #sleepCalled = true but still running. Either await the result and reset the flag on failure, or wrap the call in a try/catch block to maintain consistency.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| // Wait for actor to fully start before stopping it to avoid race conditions | ||
| if (actor.loadPromise) await actor.loadPromise.catch(); | ||
| if (actor.startPromise?.promise) await actor.startPromise.promise.catch(); | ||
| if (actor.writePromise) await actor.writePromise.catch(); |
There was a problem hiding this comment.
Race condition risk: There's a potential race condition in the sleep sequence. While the code waits for existing promises to complete, new operations could start between these awaits completing and the actor being marked as removed on line 303. Consider adding a flag at the beginning of the method to prevent new operations from starting, or implementing a lock mechanism to ensure atomic transitions to the sleeping state. This would prevent operations from continuing on a sleeping actor and avoid potential state corruption.
| // Wait for actor to fully start before stopping it to avoid race conditions | |
| if (actor.loadPromise) await actor.loadPromise.catch(); | |
| if (actor.startPromise?.promise) await actor.startPromise.promise.catch(); | |
| if (actor.writePromise) await actor.writePromise.catch(); | |
| // Mark actor as sleeping to prevent new operations from starting | |
| actor.isSleeping = true; | |
| // Wait for actor to fully start before stopping it to avoid race conditions | |
| if (actor.loadPromise) await actor.loadPromise.catch(); | |
| if (actor.startPromise?.promise) await actor.startPromise.promise.catch(); | |
| if (actor.writePromise) await actor.writePromise.catch(); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
NathanFlurry
force-pushed
the
08-17-feat_core_actor_sleeping
branch
from
620dda3 to
804497f
Compare
|
Claude finished @NathanFlurry's task —— View job Code Review: Actor Sleeping Feature Todo List:
OverviewThis PR introduces an actor sleeping mechanism that allows inactive actors to be put into a dormant state to conserve resources. The feature includes:
✅ Critical Issue ResolvedTimer Management Bug FixedThe timer management issue flagged by previous reviews has been resolved. Line 1522 in 🏗️ Architecture & Design✅ Excellent Design Decisions
Driver ImplementationsEngine Driver (packages/core/src/drivers/engine/actor-driver.ts:352-354):
File System Driver (packages/core/src/drivers/file-system/actor.ts:82-84):
🧪 Test Coverage - Outstanding✅ Comprehensive Test Suite AddedThe PR includes excellent test coverage with: New Test Fixtures (
Integration Tests (
Test Fixture Improvements:
🔒 Security Analysis✅ No security concerns identified:
🚀 Performance Implications✅ Excellent Performance Benefits
Performance Considerations
📋 Code Quality & Best Practices✅ Outstanding Implementation Quality
Sleep Logic ImplementationThe
|
Merge activity
|
1 participant
Fixes KIT-198