feat(rivetkit): add ability to destroy actors #3458
Conversation
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/limits#rate-limits |
PR Review: Add Ability to Destroy ActorsSummaryThis PR adds actor destruction functionality to RivetKit, allowing actors to be properly destroyed and resurrected with generation tracking. The implementation is comprehensive and well-tested, but there are several areas that need attention. Critical Issues1. Race Condition in Destroy Logic (High Priority)Location: The startDestroy(actorId: string): void {
const handler = this.#globalState.actors.get(doId);
// Actor not loaded, nothing to destroy
if (!handler || !handler.actor) {
return; // ⚠️ Actor exists in storage but isn't loaded - silent failure
}
// Check if already destroying
if (handler.destroying) {
return;
}
handler.destroying = true;
// ...
}Problem: If an actor exists in Durable Object storage but hasn't been loaded into memory yet, the destroy operation silently fails. This could lead to:
Recommendation: Either:
2. Missing Error Handling in Background KV Deletion (Medium Priority)Location: doState.ctx.waitUntil(
env.ACTOR_KV.delete(GLOBAL_KV_KEYS.actorMetadata(actorId)),
);Problem: The background KV deletion has no error handling. If this fails:
Recommendation: Add error handling and logging: doState.ctx.waitUntil(
env.ACTOR_KV.delete(GLOBAL_KV_KEYS.actorMetadata(actorId))
.catch(err => {
this.#log.error({
msg: "failed to delete actor metadata from KV",
actorId,
error: stringifyError(err),
});
})
);3. Generation Mismatch Error Handling (Medium Priority)Location: if (generation !== expectedGeneration) {
throw new Error(
`Actor ${actorId} generation mismatch - expected ${expectedGeneration}, got ${generation}`,
);
}Problem: This throws a generic
Recommendation: Create a specific error type: export class ActorGenerationMismatch extends ActorError {
constructor(actorId: string, expected: number, actual: number) {
super(
"actor",
"generation_mismatch",
`Actor ${actorId} generation mismatch - expected ${expected}, got ${actual}`,
{ public: true, metadata: { actorId, expected, actual } }
);
}
}Code Quality Issues4. Inconsistent SQL Schema Comments (Low Priority)Location: The SQL table creation comments are helpful, but the schema definition could be clearer: CREATE TABLE IF NOT EXISTS _rivetkit_metadata(
id INTEGER PRIMARY KEY CHECK (id = 1), // Not obvious why id=1 constraint
name TEXT NOT NULL,
key TEXT NOT NULL,
destroyed INTEGER DEFAULT 0,
generation INTEGER DEFAULT 0
);Recommendation: Add a comment explaining the id=1 constraint ensures single-row table. 5. Potential Memory Leak in Global State (Medium Priority)Location: #dos: Map<string, DurableObjectGlobalState> = new Map();
#actors: Map<string, ActorHandler> = new Map();Problem: The Recommendation: Consider cleanup strategy or document the lifecycle expectations. 6. Type Safety in SQL Queries (Low Priority)Location: Multiple files using The SQL queries use type assertions without validation: const name = result.value[0] as string;
const key = JSON.parse(result.value[1] as string) as ActorKey;
const destroyed = result.value[2] as number;
const generation = result.value[3] as number;Recommendation: Add runtime validation for critical fields: invariant(typeof name === 'string', 'name must be string');
invariant(typeof destroyed === 'number', 'destroyed must be number');Security Concerns7. Actor Key Storage Security (Low Priority)Location: Actor keys are stored as JSON strings in SQLite: key TEXT NOT NULL,
// ...
const key = JSON.parse(result.value[1] as string) as ActorKey;Analysis: This is acceptable since:
Recommendation: Add a comment or documentation noting that actor keys should not contain sensitive information. Performance Considerations8. Serial KV Operations (Medium Priority)Location: async kvBatchPut(actorId: string, entries: [Uint8Array, Uint8Array][]): Promise<void> {
const sql = this.#getDOCtx(actorId).storage.sql;
for (const [key, value] of entries) {
kvPut(sql, key, value); // Sequential INSERTs
}
}Problem: Each Recommendation: Use a single SQL transaction with multiple inserts: sql.exec('BEGIN TRANSACTION');
try {
for (const [key, value] of entries) {
kvPut(sql, key, value);
}
sql.exec('COMMIT');
} catch (err) {
sql.exec('ROLLBACK');
throw err;
}9. Full Table Scan in kvListPrefix (High Priority)Location: export function kvListPrefix(sql: SqlStorage, prefix: Uint8Array): [Uint8Array, Uint8Array][] {
const cursor = sql.exec("SELECT key, value FROM _rivetkit_kv_storage"); // ⚠️ Full table scan!
const entries: [Uint8Array, Uint8Array][] = [];
for (const row of cursor.raw()) {
const key = toUint8Array(row[0]);
const value = toUint8Array(row[1]);
if (hasPrefix(key, prefix)) { // Filtering in application code
entries.push([key, value]);
}
}
return entries;
}Problem: This does a full table scan and filters in memory. For actors with many KV entries, this is very inefficient. Recommendation: Unfortunately, SQLite doesn't support BLOB prefix matching well. Consider:
Test Coverage10. Excellent Test Coverage ✅The test suite (
Minor Suggestion: Add test cases for:
Best Practices & Style11. Good Use of Generation Tracking ✅The generation tracking pattern is excellent:
Observation: The 12. Logging Consistency (Low Priority)Most logging follows the structured logging pattern correctly: logger().debug({ msg: "actor creating", key: req.key, generation });However, some logs could include more context for debugging destroyed actors. Documentation13. Missing Documentation (Medium Priority)Missing:
Recommendation: Add JSDoc comments to key methods and a README section explaining:
Breaking Changes14. Wrangler Configuration Change (Breaking Change)Location: -"new_classes": ["ActorHandler"]
+"new_sqlite_classes": ["ActorHandler"]Impact: This is a breaking change for existing deployments. Users will need to update their wrangler.json files. Recommendation:
SummaryStrengths:
Must Fix Before Merge:
Should Fix:
Nice to Have:
Overall, this is a solid implementation. The core functionality is well-designed, but the race condition and performance issues need to be addressed before merging. Estimated Time to Fix Critical Issues: 2-3 hours |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
PR Review: Actor Destroy FunctionalitySummaryThis PR adds the ability to destroy actors in RivetKit, introducing a generation-based tracking system for Cloudflare Workers actors to handle actor lifecycle management including resurrection. The implementation is comprehensive with good test coverage. 🎯 StrengthsArchitecture & Design
Testing
🐛 Potential IssuesCritical
Moderate
Minor
⚡ Performance Considerations
🔒 Security Considerations
📝 Code Quality & Best PracticesPositive
Suggestions
🧪 Test CoverageExcellent coverage with:
Missing test cases to consider:
📋 RecommendationsHigh Priority
Medium Priority
Low Priority
✅ VerdictThis is a well-designed feature with good architecture and comprehensive testing. The generation-based tracking is a clever solution for handling actor resurrection. The critical issues identified are mostly around edge cases and performance optimization. Recommendation: Approve with requested changes to address the race condition and performance issues. The memory leak and error handling should be fixed before merge. Great work overall! 🚀 |
NathanFlurry
force-pushed
the
11-12-feat_rivet-engine_udb_key_parser
branch
from
72318fc to
cc00181
Compare
NathanFlurry
force-pushed
the
11-12-feat_rivetkit_add_ability_to_destroy_actors
branch
from
892e9cb to
90de47a
Compare
PR Review: Add ability to destroy actorsSummaryThis PR implements actor destruction functionality for RivetKit, adding the ability to explicitly destroy actors and clean up their state. The implementation includes generation tracking to handle actor resurrection and prevent stale references. Code Quality & ArchitectureStrengths
Areas for Improvement1. Race condition in actor loading (rivetkit-typescript/packages/cloudflare-workers/src/actor-driver.ts:132-160)// Check if actor is already loaded
let actorState = this.#globalState.getActorState(doState.ctx);
if (actorState?.actorInstance) {
return actorState.actorInstance;
}
// Create new actor state if it doesn't exist
if (!actorState) {
actorState = new ActorGlobalState();
actorState.actorPromise = promiseWithResolvers();
this.#globalState.setActorState(doState.ctx, actorState);
}
// Another request is already loading this actor, wait for it
if (actorState.actorPromise) {
await actorState.actorPromise.promise;
// ...
}Issue: There's a potential race between checking Recommendation: Initialize the promise immediately when creating actorState: if (!actorState) {
actorState = new ActorGlobalState();
actorState.actorPromise = promiseWithResolvers();
this.#globalState.setActorState(doState.ctx, actorState);
} else if (actorState.actorPromise) {
// Already loading
await actorState.actorPromise.promise;
return actorState.actorInstance!;
}
// Continue with loading...2. Generation validation inconsistency (rivetkit-typescript/packages/cloudflare-workers/src/actor-driver.ts:178-183)// Check if generation matches
if (generation !== expectedGeneration) {
throw new Error(
`Actor ${actorId} generation mismatch - expected ${expectedGeneration}, got ${generation}`,
);
}Issue: This throws a generic Recommendation: Use if (generation !== expectedGeneration) {
throw new ActorNotFound(actorId);
}3. Missing error handling in async destroy (rivetkit-typescript/packages/cloudflare-workers/src/actor-driver.ts:295-320)async #callOnStopAsync(
actorId: string,
doId: string,
actor: CoreAnyActorInstance,
) {
// Stop
await actor.onStop("destroy");
// ... cleanup
}Issue: There's no try-catch around the destroy logic. If Recommendation: Add error handling and logging: async #callOnStopAsync(
actorId: string,
doId: string,
actor: CoreAnyActorInstance,
) {
try {
await actor.onStop("destroy");
const doState = this.#globalState.getDOState(doId);
const sql = doState.ctx.storage.sql;
sql.exec("UPDATE _rivetkit_metadata SET destroyed = 1 WHERE 1=1");
sql.exec("DELETE FROM _rivetkit_kv_storage");
await doState.ctx.storage.deleteAlarm();
const env = getCloudflareAmbientEnv();
doState.ctx.waitUntil(
env.ACTOR_KV.delete(GLOBAL_KV_KEYS.actorMetadata(actorId)),
);
const actorHandle = this.#globalState.getActorState(doState.ctx);
actorHandle?.reset();
} catch (error) {
logger().error({ msg: "error during actor destroy", actorId, error });
// Consider: should we still mark as destroyed? Or roll back?
}
}4. SQL injection vulnerability (rivetkit-typescript/packages/cloudflare-workers/src/actor-kv.ts)While reviewing the diff, I notice SQL execution with blob data. Ensure all SQL queries use parameterized queries, not string concatenation. The implementation appears to use proper parameterization, but verify this throughout. 5. Unclear destroy vs destroyed state (rivetkit-typescript/packages/cloudflare-workers/src/actor-driver.ts:85-89)/**
* Indicates if `startDestroy` has been called.
*
* This is stored in memory instead of SQLite since the destroy may be cancelled.
*
* See the corresponding `destroyed` property in SQLite metadata.
*/
destroying: boolean = false;Question: The comment mentions "destroy may be cancelled" but there's no cancellation logic in the implementation. Is this intended for future functionality? If not, this comment is misleading. Performance Considerations
Security Concerns
Test CoverageThe test suite in
Missing tests:
Minor Issues
export class RequestHandlerNotDfeined extends ActorError {Should be
Recommendations SummaryHigh Priority:
Medium Priority: Low Priority: Overall AssessmentThis is a well-architected implementation of actor destruction with thoughtful consideration for generation tracking and state cleanup. The core logic is solid, but would benefit from additional error handling and edge case tests. The SQL migration is a good architectural choice for blob key support. Recommendation: Approve with suggested improvements, particularly around error handling and race conditions. 🤖 Generated with Claude Code |
NathanFlurry
force-pushed
the
11-12-feat_rivetkit_add_ability_to_destroy_actors
branch
from
90de47a to
6cd3c7d
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/limits#rate-limits |
Pull Request Review: Actor Destruction FeatureI've completed a comprehensive review of this PR that adds actor destruction capabilities to RivetKit. Overall, this is a well-implemented feature with good architectural decisions. Below are my findings organized by category. ✅ Strengths1. Excellent Generation-Based ArchitectureThe generation counter system is a clean solution for handling actor resurrection:
2. Proper State ManagementThe separation of concerns between memory and persistent state is well-designed:
3. Good Test CoverageThe PR includes driver tests with proper fixtures (destroy.ts, driver-tests.test.ts) that validate the destroy functionality. 4. Cloudflare Workers Migration FixThe wrangler.json fix changing 🔴 Critical Issues1. Race Condition in
|
| Category | Rating | Notes |
|---|---|---|
| Architecture | ⭐⭐⭐⭐⭐ | Generation-based resurrection is excellent |
| Error Handling | ⭐⭐⭐ | Missing some edge cases |
| Performance | ⭐⭐⭐⭐ | Good, but kvListPrefix needs optimization |
| Security | ⭐⭐⭐⭐ | Generally safe, watch for SQL performance issues |
| Test Coverage | ⭐⭐⭐⭐ | Good test fixtures included |
| Documentation | ⭐⭐⭐ | Code is readable but could use more inline comments |
🎯 Recommendations Summary
Must Fix Before Merge:
- Fix race condition in
startDestroy - Add destroying check in
loadActor - Optimize
kvListPrefixto avoid loading all rows
Should Fix:
4. Add try-finally to #callOnStopAsync
5. Fix parseActorId to handle colons in DO IDs
6. Add defensive deletion on resurrection
Nice to Have:
7. Add more debug logging
8. Improve inline documentation
9. Add SQL comments for schema
Conclusion
This is a solid implementation of actor destruction with a clever generation-based resurrection system. The main concerns are around race conditions during destroy and performance of the KV list operation. Once the critical issues are addressed, this will be a robust feature.
Great work on the architecture! The generation counter approach is particularly elegant. 👏
Pull Request Review: Actor Destroy FunctionalityThank you for this comprehensive PR adding actor destroy functionality to RivetKit! This is a well-structured implementation with good test coverage. Below is my detailed review: ✅ Strengths1. Architecture & Design
2. State Management
3. Test Coverage
4. Code Quality
🔧 Issues & ConcernsCritical Issues1. Race Condition in
|
NathanFlurry
force-pushed
the
11-12-feat_rivetkit_add_ability_to_destroy_actors
branch
from
6cd3c7d to
3985a9e
Compare
NathanFlurry
force-pushed
the
11-12-feat_rivet-engine_udb_key_parser
branch
from
cc00181 to
3777832
Compare
Code Review: Actor Destroy FeatureGreat work on implementing the actor destroy functionality! This is a solid implementation with good test coverage. Strengths
Potential Issues1. Race Condition in Destroy Cleanup (High Priority) The callOnStopAsync method performs cleanup asynchronously but doesn't prevent concurrent access during cleanup. If a request arrives after startDestroy is called but before onStop completes, the actor might still be loaded and used. The destroying flag is checked in getForId but loadActor doesn't check this flag. Recommendation: Add a check in loadActor to prevent loading actors that are marked as destroying. 2. KV Prefix List Performance Issue (Medium Priority) The kvListPrefix function performs a full table scan and checks prefixes in memory. This will be slow for actors with many KV entries. Consider using SQLite BLOB comparisons for more efficient prefix matching. 3. Missing Invariant Check in Actor ID Parsing (Low Priority) The parseActorId function doesn't use the project's error system. Use the invariant helper or a custom error for consistency. Security
Performance
Test CoverageComprehensive test suite includes destroy scenarios with/without connections, state persistence, and resurrection. Suggestion: Add a test for the race condition scenario. SummaryThis is a well-implemented feature with good architecture. Main concerns:
Recommendation: Address the high-priority race condition before merging. Other items can be addressed in follow-up PRs if preferred. |
PR Review: Actor Destroy FeatureOverviewThis PR adds the ability to destroy actors in RivetKit, implementing generation-based actor lifecycle management. The implementation is comprehensive and well-structured. StrengthsArchitecture & Design✅ Generation-based resurrection: The generation counter approach is excellent for preventing stale references Critical Issue: Race Condition in startDestroyLocation: actor-driver.ts:273-294 If startDestroy is called before an actor is loaded, it returns early without marking anything. A subsequent loadActor call could then load the actor after destroy was requested. Scenario:
Recommendation: Store a pending destroy flag that loadActor checks, or mark the actor as destroyed in SQL before attempting to load it. Medium Issues
Low Priority Issues
Testing SuggestionsConsider adding tests for:
DocumentationSummaryThis is a well-designed feature with solid code quality. The generation-based approach is excellent. However, the race condition in startDestroy should be addressed before merging. RecommendationsBefore merge:
Post-merge:
Overall: Approve with requested changes 🚀 |
No description provided.