chore(rivetkit): remove legacy behavior#4282
Conversation
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4282
|
🚅 Deployed to the rivet-pr-4282 environment in rivet-frontend
|
How to use the Graphite Merge QueueAdd 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. |
PR Review:
|
| Prefix | Old | New |
|---|---|---|
| MESSAGES | 3 | (removed) |
| WORKFLOW | 4 | 3 |
| ENTRY_METADATA | 5 | 4 |
Any actor with persisted workflow state before this change will silently read the wrong data after the migration. When loadStorage tries to read workflow metadata at [3, ...], it will find old message data at that prefix (for any existing persisted actors). When it reads entry metadata at [4, ...], it will find old workflow metadata instead.
There is no migration code to handle existing persisted data. If there is any production deployment of the workflow engine, this needs either:
- A migration that reads and re-writes the old keys at the new locations, or
- Explicit documentation that this is a clean-break requiring a data wipe
Bug: InMemoryDriver.waitForMessages Fallback Uses Fragile Type Cast
// testing.ts
const driver = this.messageDriver as WorkflowMessageDriver & {
waitForMessages?: (messageNames: string[], abortSignal: AbortSignal) => Promise<void>;
};
if (driver.waitForMessages) {
await driver.waitForMessages(messageNames, abortSignal);
return;
}WorkflowMessageDriver doesn't declare waitForMessages, so this duck-types at runtime. This is especially fragile given that waitForMessages was just made required on EngineDriver. Consider adding waitForMessages? to WorkflowMessageDriver as an optional method, or splitting into a separate interface, so the capability can be expressed in the type system.
Breaking: deserializeWorkflowError Legacy Format Removed Without Notice
The old implementation gracefully handled two legacy formats:
- A plain CBOR string (
typeof decoded === "string") - A legacy text-encoded error (fallback in the
catchblock)
Both fallbacks are now removed:
// Old
try {
const decoded = cbor.decode(bytes);
if (typeof decoded === 'string') {
return { name: 'Error', message: decoded };
}
// ...
} catch {
const message = new TextDecoder().decode(bytes);
return { name: 'Error', message };
}
// New — throws on both legacy formats
const decoded = cbor.decode(bytes);
assertObject(decoded, 'WorkflowError');Any workflow that has a persisted error value in either of those legacy formats will throw on deserialization. If those formats were never used in production, this is fine—but it should be explicitly documented.
Test Behavior Change Needs Clarification
The test "should not see messages sent during execution until next run" was renamed to "should consume messages sent during execution" and the yield-mode branch was removed:
// Old test had this yield-mode guard:
if (mode === 'yield') {
const result1 = await handle.result;
expect(result1.state).toBe('sleeping');
const result2 = await runWorkflow(...).result;
expect(result2.state).toBe('completed');
return;
}This was documenting that in yield mode, mid-execution messages required a re-run to be visible (because they were loaded from KV at the start of each run). Removing this changes the semantics of yield mode—messages sent mid-execution are now visible in the same run via the in-memory driver.
This is likely the intended behavioral change, but it should be explicitly called out in the PR description since it affects how yield-mode workflows can be used.
Minor: Agent-Generated Files in PR
Two files appear to be AI agent working files included by mistake:
.agent/notes/actor-kv-issues-and-sqlite-resolution.md— a detailed audit notes file- The CLAUDE.md change adds
## Notes Tracking: When the user asks to track something in a note, store it in.agent/notes/`` — an agent instruction rather than project documentation
The ACTOR_KV_STRUCTURE.md and the substantive CLAUDE.md raw KV limits section are genuinely useful additions and should be kept.
Positive Observations
- Simplifying
WorkflowMessageDriverby making all methods required and removing the KV-backed fallback is a clean API improvement. - The
InMemoryWorkflowMessageDriverintesting.tsis well-implemented: event-based waiters, correct abort signal cleanup, and proper completable semantics. - The architecture documentation update in
architecture.mdclearly explains the new message delivery model. - The
ActorWorkflowMessageDriversimplification (removing theloadMessagesstub that returned[]) is a good cleanup. - Test helper
queueMessage()is much cleaner than the old raw KV set pattern.
Summary
The core changes are architecturally sound, but this PR needs attention on the key prefix renumbering before merging to any environment with persisted workflow data. The legacy error format removal and the waitForMessages type cast should also be addressed.
1 participant
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: