chore(rivetkit): move conns to separate persisted kv keys #3420
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Pull Request Review: Move Connections to Separate Persisted KV KeysSummaryThis PR refactors the actor persistence layer to store connection data in separate KV entries instead of bundling everything into a single actor state blob. This is a significant architectural improvement with good design choices. Positive HighlightsArchitecture & Design
Code Quality
Issues & Concerns🔴 Critical: Race Condition in Connection RemovalLocation: #removeConn(conn: Conn<S, CP, CS, V, I, DB>) {
// Remove from KV directly
this.#removeConnFromKV(conn.id).catch((error) => {
this.#rLog.error({
msg: "failed to remove connection from KV",
connId: conn.id,
error: stringifyError(error),
});
});
// Connection is now removed from KV storage only
// Remove from state and tracking
this.#connections.delete(conn.id);
this.#changedConnections.delete(conn.id);Problem: The KV deletion is fire-and-forget with
Recommendation: Make this async and await the deletion: async #removeConn(conn: Conn<S, CP, CS, V, I, DB>) {
// Remove from KV storage first
try {
await this.#removeConnFromKV(conn.id);
} catch (error) {
this.#rLog.error({
msg: "failed to remove connection from KV",
connId: conn.id,
error: stringifyError(error),
});
throw error; // Or decide on proper error handling
}
// Only remove from memory after successful KV deletion
this.#connections.delete(conn.id);
this.#changedConnections.delete(conn.id);
// ... rest of cleanup
}🟡 Potential Issue: Missing Error RecoveryLocation: The
Recommendation: Consider adding:
🟡 Schema Naming InconsistencyLocation: The schema types changed from Recommendation: Either:
🟡 Type Safety: Unused TextDecoderLocation: async kvBatchPut(
actorId: string,
entries: [Uint8Array, Uint8Array][],
): Promise<void> {
const storage = this.#getDOCtx(actorId).storage;
const encoder = new TextDecoder(); // ⚠️ Never usedThe 🟡 Performance: Missing Index for Prefix SearchesLocation: The for (const kvEntry of entry.state.kvStorage) {
const keyBytes = new Uint8Array(kvEntry.key);
// Check if key starts with prefix
if (keyBytes.length >= prefix.length) {
let hasPrefix = true;
for (let i = 0; i < prefix.length; i++) {
if (keyBytes[i] !== prefix[i]) {
hasPrefix = false;
break;
}
}Impact: O(n*m) complexity where n = number of keys, m = prefix length. For actors with many connections, this could be slow. Recommendation:
Minor IssuesDocumentation
Code Style
Testing
Performance ConsiderationsPositive Impact:
Potential Concerns:
Security Assessment
RecommendationsMust Fix (Before Merge)
Should Fix
Nice to Have
Overall AssessmentThis is a well-thought-out refactoring that improves the architecture by separating connection state from actor state. The code quality is generally high with good error handling and logging. The main concern is the fire-and-forget deletion in Recommendation: Request changes for the critical race condition fix, then approve after addressing. |
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
NathanFlurry
force-pushed
the
11-08-chore_rivetkit_move_conns_to_separate_persisted_kv_keys
branch
from
ae73e44 to
eb0e6f2
Compare
Code Review - PR #3420: Move conns to separate persisted kv keysSummaryThis PR refactors RivetKit's connection storage from embedding all connection data in a single persisted actor state to using separate KV entries per connection. This is a significant architectural improvement that enhances scalability and performance. Strengths
Issues & ConcernsCritical Issues
Performance Concerns
Code Quality Issues
Security Considerations
Testing Recommendations
Minor Issues
Questions
VerdictOverall this is a well-architected improvement to RivetKit's persistence layer. The main concerns are:
Once the critical issues are addressed, this will be a solid enhancement. 🤖 Review generated with Claude Code |
Pull Request Review: Move Connections to Separate Persisted KV KeysThis PR introduces a significant architectural change by moving connection data from a single monolithic persisted data structure to separate KV keys. I've reviewed the changes and identified several areas for improvement along with some potential issues. OverviewThe PR changes the persistence model from storing all actor state (including connections) in a single Critical Issues1. Cloudflare Driver Key Encoding InconsistencyLocation: The #uint8ArrayToKey(key: Uint8Array): string {
if (key.length > 0 && key[0] === 2) {
const connId = new TextDecoder().decode(key.slice(1));
return `${KEYS.CONN_PREFIX}${connId}`;
}
return KEYS.PERSIST_DATA;
}Issues:
Recommendation: #uint8ArrayToKey(key: Uint8Array): string {
if (key.length === 0) {
throw new Error("Empty key provided");
}
if (key[0] === 2) {
// Connection key - extract connId
try {
const connId = new TextDecoder("utf-8", { fatal: true }).decode(key.slice(1));
return `${KEYS.CONN_PREFIX}${connId}`;
} catch (error) {
throw new Error(`Failed to decode connection ID: ${error}`);
}
} else if (key[0] === 1) {
// Persist data key
return KEYS.PERSIST_DATA;
} else {
throw new Error(`Unknown key prefix: ${key[0]}`);
}
}2. Schema Version Not Added to Build ScriptLocation: The build script was updated to include
Recommendation: Add migration documentation or upgrade logic if needed. 3. Potential Race Condition in Connection State SavingLocation: The connection change tracking uses
If
Recommendation: Review the synchronization model and ensure the Medium Priority Issues4. Missing Error Handling in Connection DecodingLocation: for (const [_key, value] of connEntries) {
try {
const conn = cbor.decode(value) as PersistedConn<CP, CS>;
connections.push(conn);
} catch (error) {
this.#rLog.error({
msg: "failed to decode connection",
error: stringifyError(error),
});
}
}Issue: When a connection fails to decode, it's silently skipped with only a log. This could lead to:
Recommendation:
5. Unbounded KV List OperationLocation: const connEntries = await this.#actorDriver.kvListPrefix(
this.#actorId,
KEYS.CONN_PREFIX,
);Issue: The
Recommendation:
6. Type Safety in Schema ConversionLocation: Throughout the schema conversion code The bare schema type definitions have been renamed (e.g., Recommendation: Consider aligning TypeScript type names with the schema names for consistency. Low Priority / Nice to Have7. Logging Inconsistencies
8. Code Duplication in Key EncodingThe key encoding logic appears in both:
These should ideally share the same encoding/decoding logic to prevent divergence. 9. Missing Test CoverageGiven the significant architectural change, I'd expect to see:
Recommendation: Add comprehensive test coverage before merging. 10. Performance ConsiderationMoving to separate keys is good for isolation, but may impact performance:
For actors with many connections, consider:
Positive Notes✅ Good architectural decision - Separating connection state from actor state improves isolation and allows for better scalability ✅ Proper use of batched KV operations - Using ✅ Change tracking - The ✅ Backward compatibility attempt - The schema versioning shows awareness of upgrade paths Recommendations SummaryBefore merging:
Future improvements: Overall, this is a solid refactoring that improves the architecture, but needs some hardening before production use. |
NathanFlurry
force-pushed
the
11-08-chore_rivetkit_remove_deprecated_conn_tokens_sse_support
branch
from
06d93f5 to
e109ee5
Compare
NathanFlurry
force-pushed
the
11-08-chore_rivetkit_move_conns_to_separate_persisted_kv_keys
branch
from
d818375 to
c57be74
Compare
Code Review: Move conns to separate persisted KV keysOverviewThis PR refactors connection persistence in RivetKit by moving connection data from a single monolithic persisted state object to individual KV entries. This is a significant architectural improvement that addresses scalability and performance concerns. Positive Aspects1. Architecture Improvement ✅
2. Clean Abstraction ✅The new KV batch operations (
3. Change Tracking ✅The proxy-based change tracking in
Issues & Concerns1. Schema Versioning & Migration
|
Merge activity
|
graphite-app
bot
deleted the
11-08-chore_rivetkit_move_conns_to_separate_persisted_kv_keys
branch
No description provided.