Uh oh!
There was an error while loading. Please reload this page.
perf: use Map for invocationsQueue (O(1) lookup/delete) - #541
Conversation
🦋 Changeset detectedLatest commit: 7a903a8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🧪 E2E Test ResultsNo test result files found.
Check the workflow run for details. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
0b839d9 to
c9cd63eCompare4c9a674 to
369a873CompareReplace array-based invocationsQueue with Map<string, QueueItem> for O(1) lookup and delete operations. This eliminates the O(n²) complexity from findIndex + splice patterns in step.ts, hook.ts, and sleep.ts. Changes: - private.ts: Change invocationsQueue type from QueueItem[] to Map - workflow.ts: Initialize with new Map() instead of [] - global.ts: Accept Map or array in WorkflowSuspension, add single-pass counting - step.ts: Use Map.set/has/delete instead of push/findIndex/splice - hook.ts: Use Map.set/delete instead of push/findIndex/splice - sleep.ts: Use Map.set/get/delete instead of push/find/findIndex/splice This is Phase 1 of the high-concurrency workflow optimization plan. See beads issue wrk-fyx for details. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
bdefce0 to
fe0a6b0CompareThere was a problem hiding this comment.
Pull request overview
This PR optimizes the invocationsQueue data structure by replacing an array-based implementation with a Map<string, QueueItem> for O(1) lookup and delete operations. This eliminates O(n²) complexity from patterns like findIndex + splice that were previously used in workflow orchestration, improving performance in high-concurrency scenarios.
Key changes:
- Changed
invocationsQueuefromQueueItem[]toMap<string, QueueItem>keyed bycorrelationId - Replaced array operations (
push,findIndex,splice,find) with Map operations (set,has,delete,get) - Updated
WorkflowSuspensionconstructor to accept Map and convert to array for backwards compatibility
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
packages/core/src/private.ts | Updated WorkflowOrchestratorContext type definition to use Map<string, QueueItem> instead of QueueItem[] |
packages/core/src/workflow.ts | Changed initialization from empty array [] to new Map() |
packages/core/src/global.ts | Modified WorkflowSuspension constructor to accept Map, convert to array, and implement single-pass counting |
packages/core/src/step.ts | Replaced push with set, findIndex+splice with has+delete |
packages/core/src/workflow/hook.ts | Replaced push with set, findIndex+splice with delete |
packages/core/src/workflow/sleep.ts | Replaced push with set, find with get, findIndex+splice with delete |
packages/core/src/step.test.ts | Updated test assertions to work with Map (using .values(), .size) |
packages/core/src/global.test.ts | Added toQueueMap helper and updated all test instantiations |
.changeset/slick-cooks-clean.md | Added changeset documenting the performance improvement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
TooTallNate
left a comment
There was a problem hiding this comment.
Left one minor suggestion. Otherwise
Co-authored-by: Nathan Rajlich <n@n8.io>
There was a problem hiding this comment.
🔧 Build Fix:
The if statement block has a misplaced closing brace with incorrect indentation that breaks the if-else structure, causing TypeScript to fail parsing the else clause and subsequent code.
View Details
📝 Patch Details
diff --git a/packages/core/src/step.ts b/packages/core/src/step.ts
index 10238dc..bcab2e1 100644
--- a/packages/core/src/step.ts+++ b/packages/core/src/step.ts@@ -75,18 +75,17 @@ export function createUseStep(ctx: WorkflowOrchestratorContext) {
if (!hasSeenStepStarted) {
// O(1) lookup and delete using Map
ctx.invocationsQueue.delete(correlationId);
- } else {- setTimeout(() => {- reject(- new WorkflowRuntimeError(- `Corrupted event log: step ${correlationId} (${stepName}) started but not found in invocation queue`- )- );- }, 0);- return EventConsumerResult.Finished;- }- hasSeenStepStarted = true;+ } else {+ setTimeout(() => {+ reject(+ new WorkflowRuntimeError(+ `Corrupted event log: step ${correlationId} (${stepName}) started but not found in invocation queue`+ )+ );+ }, 0);+ return EventConsumerResult.Finished;
}
+ hasSeenStepStarted = true;
// If this is a subsequent "step_started" event (after a retry), we just consume it
// without trying to remove from the queue again or logging a warning
return EventConsumerResult.Consumed;
Analysis
Indentation error in if-else statement causes TypeScript compilation failure
What fails: TypeScript compiler fails on packages/core/src/step.ts due to incorrect indentation in an if-else statement structure
How to reproduce:
cd packages/core && pnpm buildResult:
src/step.ts(95,9): error TS1005: ',' expected.
src/step.ts(128,8): error TS1005: ',' expected.
src/step.ts(160,1): error TS1128: Declaration or statement expected.Details: The if (!hasSeenStepStarted) block at line 75-89 had an extra closing brace on line 81 with incorrect indentation, causing the else clause on line 82 to be syntactically malformed. This caused the parser to interpret the structure incorrectly, resulting in cascading syntax errors on subsequent lines. The fix removes the misplaced brace and corrects the indentation to match the proper if-else structure.
| ); | ||
| if (invocationsQueueIndex !== -1) { | ||
| ctx.invocationsQueue.splice(invocationsQueueIndex, 1); | ||
| // O(1) lookup and delete using Map |
There was a problem hiding this comment.
There's a syntax error in the step handler: an orphaned else clause on line 78 appears after a statement rather than an if block. Additionally, the logic is broken - the error handling code (lines 79-86) is now unreachable because the delete() return value is not checked.
View Details
📝 Patch Details
diff --git a/packages/core/src/step.ts b/packages/core/src/step.ts
index 10238dc..f76f438 100644
--- a/packages/core/src/step.ts+++ b/packages/core/src/step.ts@@ -74,8 +74,8 @@ export function createUseStep(ctx: WorkflowOrchestratorContext) {
// Step has started - so remove from the invocations queue (only on the first "step_started" event)
if (!hasSeenStepStarted) {
// O(1) lookup and delete using Map
- ctx.invocationsQueue.delete(correlationId);- } else {+ const wasDeleted = ctx.invocationsQueue.delete(correlationId);+ if (!wasDeleted) {
setTimeout(() => {
reject(
new WorkflowRuntimeError(
Analysis
Syntax error in step handler due to incomplete refactoring of Map.delete()
What fails: The code in packages/core/src/step.ts (lines 75-89) contains a syntax error where an orphaned else clause appears after a statement rather than an if block, preventing the entire module from compiling.
How to reproduce:
cd packages/core
pnpm typecheckResult: TypeScript compiler reports:
src/step.ts(95,9): error TS1005: ',' expected.
src/step.ts(128,8): error TS1005: ',' expected.
src/step.ts(160,1): error TS1128: Declaration or statement expected.
Root cause: A refactoring commit (fe0a6b0) changed from checking if (ctx.invocationsQueue.has(correlationId)) before deleting to using Map.delete() directly, but the refactoring was incomplete - the inner if check was removed while leaving the corresponding else clause orphaned:
// BROKEN - orphaned else with no matching ifif(!hasSeenStepStarted){ctx.invocationsQueue.delete(correlationId);// Statement, not an if}else{// Syntax error: else without if// error handling}}Fix: Check the return value of Map.delete() (which returns boolean) to determine if the deletion succeeded:
constwasDeleted=ctx.invocationsQueue.delete(correlationId);if(!wasDeleted){// error handling}This preserves the original logic while maintaining O(1) complexity with Map operations.
Replace array-based invocationsQueue with Map<string, QueueItem> for
O(1) lookup and delete operations. This eliminates the O(n²) complexity
from findIndex + splice patterns in step.ts, hook.ts, and sleep.ts.
Changes:
This is Phase 1 of the high-concurrency workflow optimization plan.
See beads issue wrk-fyx for details.
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com