Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

Copy link
Member Author

NathanFlurry commented Jan 22, 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


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to 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.

@claude
Copy link

claude bot commented Jan 22, 2026

PR Review: feat(rivetkit): integrate workflows into actors

Summary

This PR integrates the workflow engine into RivetKit actors, allowing developers to use actor({ run: workflow(async ctx => { ... }) }) syntax. The implementation is well-architected with proper separation of concerns, comprehensive testing, and good adherence to the project's conventions.

Positive Highlights

Architecture & Design

  • Clean abstraction layers: ActorWorkflowDriver and ActorWorkflowContext provide excellent separation between workflow engine and actor runtime
  • Deterministic execution: The guard system preventing state access outside steps is clever and prevents non-deterministic bugs
  • Live mode integration: Using mode: "live" keeps workflows responsive to messages and alarms without constant polling
  • Comprehensive test coverage: Test fixtures cover basic operations, queue integration, and state guards

Code Quality

  • Type safety: Proper generic constraints maintain type safety across the stack
  • Error handling: Graceful error handling with proper cleanup (e.g., consumeGuardViolation())
  • Resource management: Good use of keepAwake() to prevent premature actor sleep during active operations

Issues & Concerns

Critical Issues

1. Inconsistent indentation in workflow/context.ts:59-64

// Current (mixed tabs/spaces):
	const stepConfig = nameOrConfig as StepConfig<T>;
	const config: StepConfig<T> = {
		...stepConfig,
		run: () => this.#withActorAccess(stepConfig.run),
	};

Issue: Line 59 uses spaces while the rest uses tabs. According to CLAUDE.md, hard tabs are required.

2. Potential race condition in QueueManager message deletion

queue-manager.ts:296-305 - Between loading messages and removal, new messages could arrive, making the index-based removal in #removeMessages potentially unsafe if other operations interleave.
Recommendation: Add a comment explaining the thread safety assumptions.

3. Empty catch block with unclear intent

workflow/context.ts:276-278 - While the comment explains intent, silently catching all errors could hide bugs (e.g., network errors during KV operations).
Recommendation: At minimum, log the error at debug level.

Major Issues

4. Memory leak potential in QueueManager listeners

queue-manager.ts:256-283 - The waitForNames implementation adds listeners but only cleans them up on resolution/rejection. If the promise never settles due to a bug, listeners accumulate.

5. Missing KV key validation

driver.ts:114-126 - The workflow driver directly constructs keys with makeWorkflowKey(key) but doesn't validate that keys stay within reasonable bounds or don't contain invalid characters.

6. Unbounded loop in sleep implementation

workflow-engine/index.ts:434-469 - If Date.now() never advances (clock issues, debugger), this could loop infinitely.
Recommendation: Add iteration limit or additional exit condition.

Minor Issues

  1. Magic number for worker poll interval (driver.ts:98) - Extract 100 to a named constant with documentation
  2. TODO.md in workflow-engine package - Should be tracked in issue tracker instead
  3. Linear search in message consumption (workflow-engine/storage.ts:379-384) - O(n) for each message, consider indexing by name for large queues

Performance Considerations

  1. Frequent KV operations - Workflow engine persists state after every operation. For high-throughput workflows, consider batching flushes.
  2. Queue message loading (queue-manager.ts:326-358) - Loads and decodes all messages every time. Consider caching for large queues.
  3. Message consumption - Linear search could be optimized with indexing for actors with many queued messages.

Security Concerns

  1. No size limits on workflow history - Could cause memory/storage exhaustion. Add configurable limits or pruning.
  2. CBOR deserialization - There is message size validation on enqueue (good), but additional runtime limits on decode could help.

Test Coverage Assessment

Strengths: Basic operations, queue integration, state guards, sleep/alarm behavior all tested

Gaps:

  • No tests for workflow rollback/error recovery
  • No tests for concurrent queue operations
  • No tests for very large queue sizes
  • Limited edge case coverage (clock skew, very long sleeps)

Recommendations by Priority

Must Fix Before Merge

  1. ✅ Fix indentation inconsistency (tabs vs spaces)
  2. ⚠️ Add error logging to empty catch blocks
  3. ⚠️ Document thread-safety assumptions in QueueManager

Should Fix Before Merge

  1. Add JSDoc to public APIs (workflow(), ActorWorkflowContext)
  2. Review and improve test coverage for rollback scenarios
  3. Add validation for workflow KV keys

Consider for Follow-up

  1. Performance optimization for large queues
  2. Workflow history pruning/limits
  3. Enhanced observability (metrics, traces)

Overall Assessment

Quality: High ⭐⭐⭐⭐☆

This is a well-designed and well-implemented feature that follows most of the project's conventions. The architecture is clean, the code is generally well-written, and the test coverage is reasonable. The critical issues are minor and easily fixable. The main areas for improvement are around error handling, performance optimization for scale, and additional test coverage for edge cases.

Recommendation: Approve with minor changes requested.


Review generated by Claude Code. Please verify all suggestions in context.

@NathanFlurry NathanFlurry force-pushed the 01-02-feat_rivetkit_workflows branch from 246d677 to a3fbdd7 Compare January 22, 2026 17:45
@NathanFlurry NathanFlurry force-pushed the 01-21-feat_rivetkit_integrate_workflows_in_to_actors branch from 2428e7f to d2ae4eb Compare January 22, 2026 17:45
@claude
Copy link

claude bot commented Jan 22, 2026

PR Review: Integrate Workflows into Actors

This PR integrates the workflow engine into RivetKit actors, enabling actors to use workflows for durable execution patterns. Overall, this is a well-designed implementation with solid test coverage.


✅ Strengths

Architecture & Design

  • Clean abstraction layer: ActorWorkflowContext wraps the workflow context and provides guard mechanisms to prevent state/vars access outside of steps
  • Proper driver implementation: ActorWorkflowDriver correctly bridges actor KV/queue operations to workflow storage
  • Namespace isolation: Workflow keys are properly prefixed to avoid collisions with actor state
  • Queue integration: Workflow messages are namespaced with __workflow: prefix

Error Handling

  • Guard violation tracking: The context tracks when state/vars are accessed outside steps and persists this in KV
  • Proper error propagation: Workflow errors are logged but don't crash the actor

Test Coverage

  • Comprehensive driver tests covering step replay, queue consumption, sleep/wake cycles, and run handler lifecycle

⚠️ Issues & Concerns

1. Indentation Inconsistency

Location: rivetkit-typescript/packages/rivetkit/src/workflow/mod.ts:59-66

The else block has inconsistent indentation that should be aligned with surrounding code.

2. clearAlarm Implementation

Location: rivetkit-typescript/packages/rivetkit/src/workflow/driver.ts:184-187

No clear alarm support exists. If a workflow sets an alarm, clears it, then sets a new one, the old alarm may still fire.

Recommendation: Implement alarm cancellation, document the limitation, or track alarm versions.

3. Workflow Lifecycle Documentation

Location: rivetkit-typescript/packages/rivetkit/src/workflow/mod.ts:33-84

Not immediately clear:

  • When the workflow is initialized
  • What happens if the workflow completes successfully
  • How eviction interacts with workflow checkpoints
  • Whether workflows can be restarted after completion

Recommendation: Add JSDoc comments explaining the lifecycle.

4. Queue Coupling

Location: rivetkit-typescript/packages/rivetkit/src/workflow/driver.ts:12-23

Workflow messages share the actor queue with a prefix, meaning:

  • Queue size limit applies to both workflow and non-workflow messages
  • Accidental sends to __workflow:foo could interfere

Recommendation: Document this clearly.

5. Test Coverage Gap

The tests don't explicitly cover:

  • Eviction during workflow step execution
  • Workflow restart after eviction mid-step
  • Rollback behavior in workflow context

6. Error Messages

Location: rivetkit-typescript/packages/rivetkit/src/workflow/context.ts:254-257

Error messages could be more helpful by explaining how to fix issues (e.g., wrap code in ctx.step()).


🔍 Code Quality

Positive Patterns

  1. Defensive coding with guard mechanism
  2. Proper cleanup with abort signals
  3. Type safety with generic constraints
  4. Clear separation of concerns

Areas for Improvement

  1. Missing comments on private methods
  2. Magic number: workerPollInterval = 100
  3. Minor style variations in async/await usage

📋 Performance Considerations

  1. KV Operations: Each workflow step triggers a KV write (ephemeral steps help)
  2. Queue Polling: Could benefit from workflow's built-in message waiting
  3. Key Prefixing Overhead: Unavoidable but worth noting

🛡️ Security Review

✅ No security concerns identified

  • Proper namespace isolation
  • No direct user input in key generation
  • Queue message validation at actor level
  • Abort signals prevent runaway workflows

📚 Documentation

Missing:

  1. Migration guide for existing actors
  2. Performance characteristics and best practices
  3. Workflow debugging guide
  4. Limitations documentation

Good:

  • ✅ specs/workflow.md added
  • ✅ Lifecycle documentation updated

🎯 Recommendations

High Priority

  1. Fix indentation in workflow/mod.ts
  2. Document or fix clearAlarm limitation
  3. Add JSDoc to main workflow function

Medium Priority

  1. Add eviction test cases
  2. Improve error messages
  3. Document queue coupling

Low Priority

  1. Add configuration for workerPollInterval
  2. Document key length limits

✨ Conclusion

Approval Status: ✅ Approve with minor changes

This is a well-designed feature with solid implementation. The main concerns are documentation and edge cases. The architecture is sound and follows RivetKit patterns correctly.

Before Merging

  • Fix indentation in workflow/mod.ts
  • Document clearAlarm limitation or implement cancellation
  • Add JSDoc to main workflow function
  • Consider adding eviction test case

Great work on this integration! 🎉

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 22, 2026

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@4006

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@4006

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@4006

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@4006

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@4006

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@4006

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@4006

@rivetkit/workflow-engine

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/workflow-engine@4006

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@4006

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@4006

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@4006

commit: 036582c

@NathanFlurry NathanFlurry force-pushed the 01-02-feat_rivetkit_workflows branch from a3fbdd7 to b4b061f Compare January 24, 2026 23:35
@NathanFlurry NathanFlurry force-pushed the 01-21-feat_rivetkit_integrate_workflows_in_to_actors branch from d2ae4eb to 2bdce1c Compare January 24, 2026 23:35
@NathanFlurry NathanFlurry force-pushed the 01-21-feat_rivetkit_integrate_workflows_in_to_actors branch from 2bdce1c to 9524546 Compare January 28, 2026 01:12
@NathanFlurry NathanFlurry force-pushed the 01-02-feat_rivetkit_workflows branch from b4b061f to 8a67460 Compare January 28, 2026 01:12
@jog1t jog1t force-pushed the 01-21-feat_rivetkit_integrate_workflows_in_to_actors branch from 9524546 to e9cb986 Compare January 28, 2026 19:55
@jog1t jog1t force-pushed the 01-02-feat_rivetkit_workflows branch from 8a67460 to a5391dd Compare January 28, 2026 19:55
@NathanFlurry NathanFlurry force-pushed the 01-21-feat_rivetkit_integrate_workflows_in_to_actors branch from e9cb986 to 7ff09e8 Compare January 30, 2026 08:19
@NathanFlurry NathanFlurry force-pushed the 01-02-feat_rivetkit_workflows branch from a5391dd to 18bb55f Compare January 30, 2026 08:19
@jog1t jog1t force-pushed the 01-02-feat_rivetkit_workflows branch from 18bb55f to 177df46 Compare January 30, 2026 21:29
@jog1t jog1t force-pushed the 01-21-feat_rivetkit_integrate_workflows_in_to_actors branch from 7ff09e8 to 036582c Compare January 30, 2026 21:29
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.

2 participants