Uh oh!
There was an error while loading. Please reload this page.
Add World.getEncryptionKeyForRun and thread encryption key through serialization - #979
Conversation
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests🌍 Community Worlds (45 failed)turso (45 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
|
🦋 Changeset detectedLatest commit: f28311e The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Uh oh!
There was an error while loading. Please reload this page.
pranaygp
left a comment
There was a problem hiding this comment.
Review: PR #979 - Add Encryptor interface and thread through serialization layer
Summary: Adds the Encryptor, EncryptionContext, and KeyMaterial interfaces to @workflow/world, makes World extend Encryptor, and threads the encryptor parameter through all serialization functions. This is a no-op refactor -- the encryptor parameter is unused (_encryptor) throughout.
Strengths:
- Clean interface design:
Encryptorhas all-optional methods, so existingWorldimplementations don't break EncryptionContextis minimal (justrunId) -- good for forward compatibilityKeyMaterialinterface for o11y tooling is a thoughtful addition- The
getEncryptorForRun()method onWorldis a well-designed escape hatch for cross-deployment encryption (e.g.,resumeHook()from newer deployment) getHookByTokenWithEncryptor()resolves the encryptor once and reuses it -- avoids redundant key resolution
Concerns:
resolveEncryptorForRuntype safety: Inresume-hook.tsline 29-31,getEncryptorForRunis accessed via(world as any).getEncryptorForRun. SinceWorldalready extendsEncryptorandgetEncryptorForRunis defined onWorld, you should be able to use optional chaining directly:world.getEncryptorForRun?.(runId). The'getEncryptorForRun' in world+as anypattern bypasses type checking unnecessarily.Serialization parameter ordering: The PR reorders parameters in the dehydrate/hydrate functions. For example,
dehydrateWorkflowArgumentsgoes from(value, ops, runId, ...)to(value, runId, encryptor, ops, ...). This is a breaking change to the internal API. While these aren't public, any external code calling these directly would break. The reorder makes sense semantically (runId + encryptor are conceptually paired), but consider documenting this in the changeset._encryptorunused parameter pattern: All 8 functions have_encryptor: Encryptorthat is unused. This is expected since the actual wiring happens in #957. However, this means if #979 lands but #957 doesn't (or is delayed), there's dead parameter threading throughout the codebase. A minor code smell but acceptable for a PR stack.hydrateResourceIOnow requiresencryptor: Inobservability.ts,hydrateResourceIOnow takes anEncryptorparameter, and all callers passworld. This means the observability layer now has a dependency on the World instance. Previously it was a pure data transformation. This is a reasonable tradeoff for encryption support, but worth noting the coupling increase.
Overall, well-structured interface design. The cross-deployment encryption support via getEncryptorForRun shows good foresight for production scenarios.
Uh oh!
There was an error while loading. Please reload this page.
f2a9138 to
78aa5f9Compare
VaguelySerious
left a comment
There was a problem hiding this comment.
One important non-blocking note on the world interface choice
| // Resolve encryption key for the new run. Since start() always runs on the | ||
| // current deployment, we can use a placeholder runId — the implementation | ||
| // will resolve to the local deployment's key regardless. | ||
| const encryptionKey = await world.getEncryptionKeyForRun?.(runId); |
There was a problem hiding this comment.
I feel like this isn't adequately documented in the world interface . If a placeholder "works", then getEncryptionKeyForRun on the world should allow null or something similar and describe how the fallback works.
There was a problem hiding this comment.
Updated the comment. The runId is a client-generated ULID that has already been created at that point — it is the actual runId that will be used for the run_created event and key derivation. Clarified the comment to explain this.
Uh oh!
There was an error while loading. Please reload this page.
pranaygp
left a comment
There was a problem hiding this comment.
Overall this is a clean, well-structured PR that threads the encryption key through the serialization layer in preparation for at-rest encryption. The interface design on World is good — optional method means no breaking changes for existing implementations. A few comments below, mostly nits and one potential performance concern.
| const startTime = Date.now(); | ||
| const stepEncryptionKey = | ||
| await world.getEncryptionKeyForRun?.(workflowRunId); | ||
| const dehydrated = await dehydrateStepReturnValue( |
There was a problem hiding this comment.
The encryption key is resolved twice for the same workflowRunId — once at line 301 for hydration and again here for dehydration. This means two calls to world.getEncryptionKeyForRun?.() per step execution.
Since workflowRunId doesn't change between hydration and dehydration, you could resolve it once before both trace blocks and reuse it:
constencryptionKey=awaitworld.getEncryptionKeyForRun?.(workflowRunId);Not a correctness issue, but for production deployments where getEncryptionKeyForRun might involve a network call (e.g., KMS), this avoids an unnecessary round-trip.
There was a problem hiding this comment.
Good catch. Hoisted the getEncryptionKeyForRun call above both trace blocks so it resolves once and is reused for both hydration and dehydration.
| export async function resumeHook<T = any>( | ||
| tokenOrHook: string | Hook, | ||
| payload: T | ||
| payload: T, |
There was a problem hiding this comment.
nit: _encryptionKey uses the underscore-prefix convention that typically signals an unused parameter, but it is used on lines 106 and 110. Consider renaming to encryptionKeyOverride or just encryptionKey to better communicate intent.
(I see @VaguelySerious flagged this too — just confirming it's still present in the latest revision.)
There was a problem hiding this comment.
Fixed — renamed to encryptionKeyOverride to properly communicate intent.
| runId: string, | ||
| _key: Uint8Array | undefined, | ||
| ops: Promise<void>[] = [], | ||
| global: Record<string, any> = globalThis, |
There was a problem hiding this comment.
nit: The JSDoc @param list above (lines 1370-1377) is stale — it still references global before runId and doesn't mention the new _key parameter. Since this is an internal API it's not urgent, but updating the params would help future readers. Same applies to the other 7 function JSDocs.
There was a problem hiding this comment.
Updated all 8 function JSDoc blocks with accurate @param descriptions matching the current signatures.
| * @returns The per-run AES-256 key, or undefined if encryption is not configured | ||
| */ | ||
| getEncryptionKeyForRun?( | ||
| run: WorkflowRun | string |
There was a problem hiding this comment.
The signature accepts WorkflowRun | string but every callsite I see in the diff passes a string (runId). Is there an existing or planned callsite that passes a full WorkflowRun object? If not, simplifying to just string would reduce the interface surface area. If there is a planned use, a brief note in the JSDoc about when to prefer which form would be helpful.
There was a problem hiding this comment.
The WorkflowRun form is used by the o11y/CLI path (in the later PRs in the stack) where the run entity is already fetched and provides the deploymentId for cross-deployment key resolution. Updated the JSDoc to document both use cases.
| resource: T, | ||
| _encryptorResolver?: unknown | ||
| ): T { | ||
| return hydrateResourceIOGeneric(resource as any, getRevivers()) as T; |
There was a problem hiding this comment.
nit: The type of _encryptorResolver is unknown here, but EncryptionKeyResolver is defined in the sibling output.ts. For consistency and forward-compat, consider importing and using that type so when encryption is wired into the CLI hydration, the type is already correct.
There was a problem hiding this comment.
Added a proper EncryptionKeyResolver type definition in hydration.ts matching the one in output.ts, and typed the parameter accordingly.
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) | Express workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) | Express workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express | Nitro Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro | Express Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) | Express Stream Benchmarks(includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
|
…e key resolution, update JSDoc params
Summary
World.getEncryptionKeyForRun(run)returningUint8Array | undefinedas the interface for retrieving per-run encryption keyskey: Uint8Array | undefined