Uh oh!
There was an error while loading. Please reload this page.
feat(core): warn on uncommitted operations when workflow completes - #1185
Conversation
🦋 Changeset detectedLatest commit: 201fb65 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests🐘 Local Postgres (1 failed)hono-stable (1 failed):
🌍 Community Worlds (46 failed)turso (46 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
❌ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
❌ Some E2E test jobs failed:
Check the workflow run for details. |
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.
05b390c to
b6e6922CompareAdd a `dispose()` method to the Hook interface that allows workflows to explicitly release hook tokens for reuse by other workflows while the current workflow is still running. This enables handoff patterns where one workflow can transfer a hook token to another workflow. - Add `dispose()` method to Hook interface in create-hook.ts - Implement dispose functionality in workflow/hook.ts - Add HookDisposedInvocationQueueItem to global.ts - Handle hook_disposed events in suspension-handler.ts - Update documentation in hooks.mdx and create-hook.mdx - Add e2e test for hook token reuse after explicit disposal https://claude.ai/code/session_01AkvrXduyFTbtV2joTPHrDH
Add Symbol.dispose to Hook interface to support the TC39 Explicit Resource Management proposal. This allows hooks to be used with the `using` keyword for automatic disposal when exiting scope. https://claude.ai/code/session_01AkvrXduyFTbtV2joTPHrDH
Update all documentation and e2e tests to use the `using` keyword as the recommended approach for creating hooks and webhooks. This leverages the TC39 Explicit Resource Management proposal for automatic disposal. - Update e2e tests to use `using` syntax - Update foundational hooks guide to recommend `using` - Update create-hook API reference with `using` examples - Update create-webhook API reference with `using` examples - Update example workflow to use `using` https://claude.ai/code/session_01AkvrXduyFTbtV2joTPHrDH
Remove unnecessary block scopes and excessive comments about automatic disposal. Block scopes are only used when early disposal is relevant (like in the handoff test). https://claude.ai/code/session_01AkvrXduyFTbtV2joTPHrDH
Add a one-liner explaining the `using` keyword in the intro examples of the API reference docs, so new users understand the syntax. https://claude.ai/code/session_01AkvrXduyFTbtV2joTPHrDH
- Restore code highlights (`[!code highlight]`) that were unintentionally
removed from pre-existing doc examples
- Move `using` explanation from prose to inline code comment in intro
examples
- Add `{/* @skip-typecheck */}` to incomplete manual dispose() snippet
- Add 409 (conflict/duplicate) error handling for hook_disposed events
in suspension handler to handle workflow re-invocation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>The workflow VM sandbox doesn't have Symbol.dispose/Symbol.asyncDispose available, causing `using` keyword to fail with "Symbol.dispose is not defined" at runtime. Add polyfill in the VM context creation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The workflow VM sandbox has its own Symbol object with a polyfilled Symbol.dispose. The hook object was using the host's Symbol.dispose, which is a different symbol instance. The SWC-compiled `using` keyword looks up the VM's Symbol.dispose on the object, causing "Object not disposable" errors. Fix by setting Symbol.dispose on the hook object dynamically using the VM's globalThis.Symbol.dispose from the orchestrator context. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The webhookWorkflow e2e test creates 3 webhooks that must all exist before the test sends HTTP requests. Using `using` with sequential creation meant only the first webhook existed at the first suspension point. Revert to `const` since all webhooks need to be created upfront. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Instead of adding a separate HookDisposedInvocationQueueItem to the queue on dispose, keep the HookInvocationQueueItem throughout the hook lifecycle and track state with flags (hasCreatedEvent, disposed). A closure variable (hasDisposedEvent) makes disposeHook() a pure no-op on replay, avoiding redundant server calls and 409 errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Match the pattern used by steps (step_completed/step_failed) and waits (wait_completed) where the queue item is removed on the terminal event. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- WorkflowSuspension counts disposed hooks separately from active hooks - Dispose after hook_created replay produces correct suspension - Dispose before first suspension (needs both create + dispose) - Multiple hooks where only one is disposed - Dispose on a conflicted hook is safe (no crash) - Symbol.dispose calls disposeHook correctly (using keyword pattern) - Iterator break without dispose keeps hook alive in queue - Await after dispose on first invocation triggers suspension Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add [Symbol.dispose] directly to the hook object so it satisfies the Hook<T> type without `as unknown as`. The VM's Symbol.dispose is still added separately when it differs from the host's. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
packages/core/src/workflow.test.ts:3943
- This test doesn't seem to exercise the "exclude hook disposals" behavior: the provided events include
hook_disposed, which removes the hook frominvocationsQueueentirely, so the queue is empty at completion regardless of any filtering inwarnPendingQueueItems(). If the intent is to assert that disposed-but-uncommitted hooks don't warn, the fixture should leave a disposed hook item in the queue (e.g., callhook.dispose()but omit thehook_disposedevent).
},
createdAt: new Date(),
},
{
eventId: 'event-2',
runId: workflowRun.runId,
eventType: 'hook_disposed',
correlationId: 'hook_01HK153X008RT6YEW43G8QX6JX',
createdAt: new Date(),
},
];
// Workflow creates hook, awaits one payload, hook is disposed
// (simulates `using` at function scope)
await runWorkflow(
`const createHook = globalThis[Symbol.for("WORKFLOW_CREATE_HOOK")];
async function workflow() {
const hook = createHook();
const result = await hook;
hook.dispose();
return result.message;
}${getWorkflowTransformCode('workflow')}`,
workflowRun,
events,
noEncryptionKey
);
const warnCalls = warnSpy.mock.calls.map((c) => c[0]);
expect(
warnCalls.some((msg: string) => msg.includes('uncommitted operation'))
).toBe(false);
} finally {
warnSpy.mockRestore();
}
});
it('should not warn when queue is empty on completion', async () => {
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
try {
const ops: Promise<any>[] = [];
const workflowRun: WorkflowRun = {
runId: 'wrun_123',
workflowName: 'workflow',
status: 'running',
input: await dehydrateWorkflowArguments(
[],
'wrun_123',
noEncryptionKey,
ops
),
createdAt: new Date('2024-01-01T00:00:00.000Z'),
updatedAt: new Date('2024-01-01T00:00:00.000Z'),
startedAt: new Date('2024-01-01T00:00:00.000Z'),
deploymentId: 'test-deployment',
};
await runWorkflow(
`function workflow() { return "clean"; }${getWorkflowTransformCode('workflow')}`,
workflowRun,
[],
noEncryptionKey
);
const warnCalls = warnSpy.mock.calls.map((c) => c[0]);
expect(
warnCalls.some((msg: string) => msg.includes('uncommitted operation'))
).toBe(false);
} finally {
warnSpy.mockRestore();
}
});
});
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Filter out hooks that are registered and alive (hasCreatedEvent && !disposed) | ||
| // — these are benign since the backend auto-disposes all hooks when a run | ||
| // reaches a terminal state | ||
| const items = [...pendingQueue.values()].filter( | ||
| (item) => !(item.type === 'hook' && item.hasCreatedEvent && !item.disposed) |
There was a problem hiding this comment.
The hook filtering logic doesn't exclude hook disposals: hooks with disposed === true (set when hook.dispose() is called but before a hook_disposed event exists) will still be reported as "uncommitted", even though the PR description says hook disposals should be excluded because the backend auto-disposes at terminal state. Consider filtering out disposed hook queue items (and/or aligning the PR description/comment with the intended behavior).
| // Filter out hooks that are registered and alive (hasCreatedEvent && !disposed) | |
| // — these are benign since the backend auto-disposes all hooks when a run | |
| // reaches a terminal state | |
| constitems=[...pendingQueue.values()].filter( | |
| (item)=>!(item.type==='hook'&&item.hasCreatedEvent&&!item.disposed) | |
| // Filter out hooks that are either registered (hasCreatedEvent) or already | |
| // disposed — these are benign since the backend auto-disposes all hooks | |
| // when a run reaches a terminal state | |
| constitems=[...pendingQueue.values()].filter( | |
| (item)=> | |
| !( | |
| item.type==='hook'&& | |
| (item.hasCreatedEvent||item.disposed) | |
| ) |
There was a problem hiding this comment.
Fixed in 201fb65 — filter now excludes hooks with hasCreatedEvent || disposed, matching TooTallNate's suggestion below.
| return `step "${item.stepName}"`; | ||
| case 'hook': | ||
| return `hook (${item.correlationId})`; | ||
| case 'wait': |
There was a problem hiding this comment.
This warning message includes the full hook token (hook (token: "...")). Hook tokens often act as shared secrets / auth credentials; logging them to console warnings can leak sensitive data. Prefer logging the hook correlationId, a redacted token (e.g., last 4 chars), or omit the token entirely.
There was a problem hiding this comment.
Went back to showing the token per TooTallNate's suggestion — this is a user-facing warning (not a server log), so the token is more meaningful than the correlationId. Fixed in 201fb65.
| const details = items.map((item) => { | ||
| switch (item.type) { | ||
| case 'step': | ||
| return `step "${item.stepName}"`; | ||
| case 'hook': | ||
| return `hook (${item.correlationId})`; | ||
| case 'wait': | ||
| return 'sleep'; | ||
| default: | ||
| return `unknown (${(item as { type: string }).type})`; |
There was a problem hiding this comment.
details relies on the switch being exhaustive. If a new QueueItem type is added later, this will silently produce undefined entries and log confusing output. Consider adding an explicit default that asserts never (or throws) so TypeScript enforces exhaustiveness here.
There was a problem hiding this comment.
Already fixed in the previous commit (86ce8f1) — there's a default case that returns unknown (${type}).
| // No step events — the unawaited step stays pending in the queue | ||
| const events: Event[] = []; | ||
| // Workflow calls step but doesn't await it, returns immediately | ||
| await runWorkflow( | ||
| `const add = globalThis[Symbol.for("WORKFLOW_USE_STEP")]("add"); | ||
| async function workflow() { | ||
| add(1, 2); // not awaited! | ||
| return "done"; | ||
| }${getWorkflowTransformCode('workflow')}`, | ||
| workflowRun, | ||
| events, | ||
| noEncryptionKey | ||
| ); | ||
| const warnCalls = warnSpy.mock.calls.map((c) => c[0]); | ||
| expect( | ||
| warnCalls.some( | ||
| (msg: string) => | ||
| msg.includes('uncommitted operation') && | ||
| msg.includes('step "add"') | ||
| ) | ||
| ).toBe(true); | ||
| expect( | ||
| warnCalls.some((msg: string) => | ||
| msg.includes('Did you forget to `await`') | ||
| ) | ||
| ).toBe(true); | ||
| } finally { | ||
| warnSpy.mockRestore(); | ||
| } | ||
| }); | ||
| it('should warn when workflow fails with pending operations', async () => { |
There was a problem hiding this comment.
This test provides a matching step_completed event for the invoked step. The step implementation deletes its invocationQueue entry on step_completed regardless of whether the returned Promise is awaited, so the queue will typically be empty by the time warnPendingQueueItems() runs and no warning will be emitted. To reliably test the new behavior, construct an event log where the step remains uncommitted at workflow completion (e.g., omit terminal events for that correlationId).
There was a problem hiding this comment.
Already fixed in 86ce8f1 — step events removed so the unawaited step stays pending.
| msg.includes('failed') && msg.includes('step "add"') | ||
| ) | ||
| ).toBe(true); | ||
| } finally { | ||
| warnSpy.mockRestore(); | ||
| } | ||
| }); | ||
| it('should not warn when only hook_disposed items remain in queue', async () => { | ||
| const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); | ||
| try { | ||
| const ops: Promise<any>[] = []; | ||
| const workflowRun: WorkflowRun = { | ||
| runId: 'test-run-123', | ||
| workflowName: 'workflow', | ||
| status: 'running', | ||
| input: await dehydrateWorkflowArguments( | ||
| [], | ||
| 'wrun_123', | ||
| noEncryptionKey, | ||
| ops | ||
| ), | ||
| createdAt: new Date('2024-01-01T00:00:00.000Z'), | ||
| updatedAt: new Date('2024-01-01T00:00:00.000Z'), | ||
| startedAt: new Date('2024-01-01T00:00:00.000Z'), | ||
| deploymentId: 'test-deployment', | ||
| }; | ||
| const events: Event[] = [ | ||
| { | ||
| eventId: 'event-0', | ||
| runId: workflowRun.runId, | ||
| eventType: 'hook_created' as const, | ||
| correlationId: 'hook_01HK153X008RT6YEW43G8QX6JX', | ||
| eventData: { | ||
| token: 'test-token', | ||
| }, | ||
| createdAt: new Date(), |
There was a problem hiding this comment.
Similar to the previous test: because the events include step_completed, the step invocation is removed from invocationsQueue even if it isn't awaited. That means the warning may not be triggered, and the test may fail intermittently depending on scheduling. Consider changing the fixture so the step is still pending/uncommitted when the workflow errors (e.g., no terminal event for the invoked correlationId).
There was a problem hiding this comment.
Already fixed in 86ce8f1 — same fix as above.
- Rename test to match new behavior (hooks stay in queue, not removed) - Use neutral "processed" verb in WorkflowSuspension message when mixed item types are present - Remove extends Disposable from Hook interface to avoid requiring lib.esnext.disposable in downstream consumers (explicit [Symbol.dispose]() method is still declared on the interface) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TooTallNate
left a comment
There was a problem hiding this comment.
Clean, focused PR. The warning is a good DX improvement that will save users debugging time. The filter logic for hook disposals is correct, and the test coverage hits the important cases. A few minor suggestions inline.
| // — these are benign since the backend auto-disposes all hooks when a run | ||
| // reaches a terminal state | ||
| const items = [...pendingQueue.values()].filter( | ||
| (item) => !(item.type === 'hook' && item.hasCreatedEvent && !item.disposed) |
There was a problem hiding this comment.
The filter condition !(item.type === 'hook' && item.hasCreatedEvent && !item.disposed) silences hooks that have been created and are actively waiting for payloads — correct, since the backend auto-disposes these.
But consider the edge case: a hook with hasCreatedEvent = undefined and disposed = undefined (i.e., a hook that was created in user code but the workflow completed before the first suspension could process it). This would not be filtered out — which is correct, since it represents a createHook() call that was never awaited. Just want to confirm this is intentional (I believe it is).
There was a problem hiding this comment.
Yes, that's intentional. A hook with hasCreatedEvent=undefined, disposed=undefined means createHook() was called but the workflow completed before any suspension — that's a forgotten await and should warn.
| // — these are benign since the backend auto-disposes all hooks when a run | ||
| // reaches a terminal state | ||
| const items = [...pendingQueue.values()].filter( | ||
| (item) => !(item.type === 'hook' && item.hasCreatedEvent && !item.disposed) |
There was a problem hiding this comment.
Nit: Also worth noting — a hook with disposed = true but hasCreatedEvent = undefined (dispose called before first suspension) will pass through the filter and trigger a warning. This seems like a false positive: the user explicitly disposed the hook. The filter might be more precise as:
(item)=>!(item.type==='hook'&&(item.disposed||(item.hasCreatedEvent&&!item.disposed)))Or equivalently, only warn for hooks that are neither disposed nor already created:
(item)=>!(item.type==='hook'&&(item.hasCreatedEvent||item.disposed))This would skip warnings for any hook the user explicitly disposed, regardless of whether it was created yet. Though in practice, disposed && !hasCreatedEvent is rare (dispose before first suspension), so this may not matter.
There was a problem hiding this comment.
Good catch — fixed in 201fb65. Filter is now \!(item.type === 'hook' && (item.hasCreatedEvent || item.disposed)), which skips warnings for any hook that's either alive or explicitly disposed.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| }); | ||
| it('should not warn when only hook_disposed items remain in queue', async () => { |
There was a problem hiding this comment.
The test name says "hook_disposed items remain in queue" but the scenario is actually: hook_disposed is in the event log, which causes the hook consumer to remove the item from the queue (see hook.ts:124). So the queue ends up empty, not with disposed items. The test is correct in behavior — it verifies no warning fires — but the name is slightly misleading. Something like "should not warn when hooks are properly disposed" might be clearer.
There was a problem hiding this comment.
Fixed in 201fb65 — renamed to "should not warn when hooks are properly disposed".
| describe('pending queue warnings', () => { | ||
| it('should warn when workflow completes with an unawaited step', async () => { | ||
| const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); |
There was a problem hiding this comment.
Suggestion: Consider extracting the WorkflowRun boilerplate into a helper at the top of this describe block, similar to how other test suites handle it. All four tests create nearly identical run objects. Something like:
functioncreateTestRun(overrides?: Partial<WorkflowRun>): WorkflowRun{ ... }Not blocking — just a readability improvement.
There was a problem hiding this comment.
Good suggestion — will leave for a follow-up since it's a readability improvement and doesn't affect behavior.
When dispose() is called while a promise is pending (e.g., iterator suspended on yield await this, or direct await hook after dispose), the promise would hang forever since the event consumer will never deliver another hook_received. Now disposeHook() clears the promises array and triggers a WorkflowSuspension so the runtime processes the disposal cleanly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Log a warning when a workflow run completes or fails with uncommitted operations still in the invocations queue (unawaited steps, hooks, or sleeps). This helps users diagnose forgotten `await` calls. Hook disposals are excluded from the warning since the backend auto-disposes all hooks when a run reaches a terminal state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test that: - Unawaited step triggers warning on successful completion - Unawaited step triggers warning on workflow failure - hook_disposed items are excluded from warning - Clean workflow completion produces no warning Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Filter out hooks with hasCreatedEvent && !disposed (alive hooks) instead of the removed hook_disposed queue item type. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove workflow from changeset (transitive via @workflow/core) - Add default case to switch for exhaustiveness - Use correlationId instead of token in warning message to avoid leaking user-provided tokens into logs - Remove step events from warning tests so unawaited steps stay pending in the queue (step_completed would clear the item during replay) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Filter out disposed hooks from warning (not just created hooks) - Show hook token instead of correlationId in warning (user-facing) - Rename test to "should not warn when hooks are properly disposed" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Log a warning when a workflow run completes or fails with uncommitted
operations still in the invocations queue (unawaited steps, hooks, or
sleeps). This helps users diagnose forgotten
awaitcalls.Hook disposals are excluded from the warning since the backend
auto-disposes all hooks when a run reaches a terminal state.
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com