fix(client): scope workflow global patching to execution context#213
fix(client): scope workflow global patching to execution context#213robelest wants to merge 2 commits intoget-convex:mainfrom
Conversation
Route Math/Date/console/fetch/timer patches through AsyncLocalStorage so they only apply during workflow execution, preventing global leakage into non-workflow code while preserving deterministic workflow behavior.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe environment patching system is refactored from direct global patching to a workflow-aware AsyncLocalStorage-backed architecture. Global patching now happens lazily via per-workflow environments, the setupEnvironment function signature is simplified, and a new runWithWorkflowEnvironment function orchestrates execution within a workflow context. Changes
Sequence DiagramsequenceDiagram
participant Client
participant WorkflowMutation
participant RunWithEnv as runWithWorkflowEnvironment
participant Storage as AsyncLocalStorage
participant WorkflowEnv as WorkflowEnvironment
participant GlobalAccessor as Global Accessor
Client->>WorkflowMutation: execute workflow
WorkflowMutation->>RunWithEnv: runWithWorkflowEnvironment(getGenerationState, workflowId, run)
RunWithEnv->>WorkflowEnv: createWorkflowEnvironment(getGenerationState, workflowId)
WorkflowEnv-->>RunWithEnv: patched Math, Date, console, etc.
RunWithEnv->>Storage: store.run(WorkflowEnv, () => ...)
Storage->>GlobalAccessor: within AsyncLocalStorage context
GlobalAccessor->>WorkflowEnv: access patched globals
WorkflowEnv-->>GlobalAccessor: deterministic/restricted values
GlobalAccessor->>Client: results with isolated environment
Storage->>Storage: cleanup context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/environment.test.ts (1)
11-17: Consider adding a comment explaining the polyfill purpose.The conditional polyfill ensures
AsyncLocalStorageis available onglobalThisfor the test environment, matching the runtime expectation inensureWorkflowEnvironmentStorage(). A brief comment would clarify this setup for future maintainers.📝 Suggested documentation
+// The workflow environment expects AsyncLocalStorage on globalThis (as provided by +// convex-backend). Polyfill it for the test environment. if ( (globalThis as { AsyncLocalStorage?: typeof AsyncLocalStorage }) .AsyncLocalStorage === undefined ) { (globalThis as { AsyncLocalStorage?: typeof AsyncLocalStorage }).AsyncLocalStorage = AsyncLocalStorage; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/environment.test.ts` around lines 11 - 17, Add a short inline comment above the polyfill block explaining that this conditional assigns AsyncLocalStorage onto globalThis for the test environment to mirror runtime expectations used by ensureWorkflowEnvironmentStorage(), so future maintainers understand it’s a deliberate test-only polyfill and not accidental global mutation; reference the AsyncLocalStorage symbol and ensureWorkflowEnvironmentStorage() in the comment for context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/environment.ts`:
- Around line 164-177: createWorkflowEnvironment currently reads globals
(global.Math, global.Date, global.console) which can be getters after
setupEnvironment runs and yield already-patched objects in nested workflow
contexts; fix this by capturing the original unpatched globals once (e.g., a
module-level ORIGINALS object populated from globalThis.Math, globalThis.Date,
globalThis.console before setupEnvironment patches anything) and change
createWorkflowEnvironment to call patchMath, createDeterministicDate, and
createConsole using those ORIGINALS (reference ORIGINALS.Math, ORIGINALS.Date,
ORIGINALS.console) instead of reading from globalThis; ensure the capture
happens before setupEnvironment runs so patchMath, createDeterministicDate, and
createConsole always wrap the true originals.
---
Nitpick comments:
In `@src/client/environment.test.ts`:
- Around line 11-17: Add a short inline comment above the polyfill block
explaining that this conditional assigns AsyncLocalStorage onto globalThis for
the test environment to mirror runtime expectations used by
ensureWorkflowEnvironmentStorage(), so future maintainers understand it’s a
deliberate test-only polyfill and not accidental global mutation; reference the
AsyncLocalStorage symbol and ensureWorkflowEnvironmentStorage() in the comment
for context.
| "AsyncLocalStorage is not available in this runtime. Update convex-backend to a build with async_hooks support.", | ||
| ); | ||
| } | ||
| workflowEnvironmentStorage = new global.AsyncLocalStorage<WorkflowEnvironment>(); |
There was a problem hiding this comment.
why using the global instead of an import?
There was a problem hiding this comment.
I kept the global path because it gives deterministic diagnostics (explicit availability check + clear error). The current node:async_hooks shim also has permissive fallbacks (createHook no-op, IDs 0) when async_hooks is absent, so it can fail less explicitly. Happy to follow up by tightening shim error behavior so import-path diagnostics match the global-path check
There was a problem hiding this comment.
Gotcha. Will this work in convex-test where our v8 shim doesn’t run? I don’t know if it’s normally a global or if we’re special-casing our own behavior
| type AsyncLocalStorageConstructor = new <T>() => AsyncLocalStorageLike<T>; | ||
|
|
||
| let workflowEnvironmentStorage: AsyncLocalStorageLike<WorkflowEnvironment> | undefined; | ||
| let globalsPatched = false; |
There was a problem hiding this comment.
why isn't this !!workflowEnvironmentsStorage? What's different?
There was a problem hiding this comment.
Storage creation and global patching are separate phases. globalsPatched makes that boundary explicit and avoids conflating partial setup with done.
…ment - Capture Math/Date/console into module-level originalGlobals before setupEnvironment runs so createWorkflowEnvironment always wraps the true originals, not getters that may return an outer workflow's patched objects in a nested execution context (CodeRabbit feedback). - Add comment explaining the globalThis.AsyncLocalStorage polyfill in environment.test.ts (CodeRabbit nitpick).
commit: |
2 participants
Summary
runWithWorkflowEnvironment(...)so deterministicMath/Dateand blockedfetch/timers only apply while a workflow is executing.Dependency
AsyncLocalStoragesupport.Validation
bun run lintbun run buildbun run testfeature/async-hooks-cped-runtime:internal.example.startWorkflow-> completed viaapi.admin.getWorkflowStatusandapi.admin.getWorkflowResultinternal.example.getGeocodingsucceeds outside workflow scopeinternal.transcription.computeEmbeddingsucceeds whenOPENAI_API_KEYis configuredBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Summary by CodeRabbit