Repository navigation
Conversation
…(breaking)
The SDK's plugin merge keeps the last Activity of each name. A caller
Activity named invokeModel, invokeModelStreaming, invokeEmbeddingModel,
<server>-listTools or <server>-callTool, a second AiSdkPlugin with
different options, or another integration's MCP Activity with the same
server name (GoogleAdkPlugin uses the same MCP names) was replaced
without a warning, and the Worker ran the wrong handler.
AiSdkPlugin.configureWorker now throws a TypeError
("Duplicate registration of Activity '<name>'") when one of its
Activities would replace a different function. Each plugin Activity
records the operation it was built for and its provider, or its MCP
client factory and idle timeout, so another operation registered under
the same name is refused. The check accepts the same plugin instance
twice, two instances with the same options, and Activities from
createActivities with the same provider. On real Workers,
createActivities next to an AiSdkPlugin starts, and so does a repeated
AiSdkPlugin on a Worker without Workflows. With workflowsPath, a
repeated AiSdkPlugin still fails Worker.create in the bundler, as it
did before this change, because each instance adds its polyfill entry.
Breaking change: Workers with such a collision used to start and now
fail in Worker.create. Rename or remove the other Activity or MCP
server. Activity names, inputs and results do not change, so recorded
histories replay unchanged.
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Review at head 211d5bb (one commit on 5e91c26; it merges cleanly with current main, whose only change to this plugin since then is #259's test helper). On a worktree at the head with pnpm 10.27.0: make lint clean, make test 34 passed and 1 skipped, including the new test. Findings were verified with probes against the built package: configureWorker chained in Worker.create's order, and real Worker.create calls against a local dev server with the plugins split between the Worker and the NativeConnection. The Strands side of those probes is a SimplePlugin registering invokeModel and invokeModelStreaming. That is all StrandsPlugin on main does with those names (strands-agents/src/plugin.ts:80-81), and it does not override configureWorker. Checked and fine: a caller Activity, a second plugin with a different provider or MCP factory, and an embedding Activity registered as invokeModel are refused as described. The same instance on the Worker and the NativeConnection, createActivities(provider) next to the plugin, and mcpConnectionIdleTimeout: '5 minutes' against the default all pass. Replay Workers are unaffected: SimplePlugin.configureReplayWorker carries no Activities, and the override is on configureWorker only. activity-sources is reached only from Worker-side modules, since the Workflow entry points import ./activities for types only. Not verified: the real StrandsPlugin class, which I simulated from its source as described above.
Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.
Where the problems cluster: the check is correct for everything registered before AiSdkPlugin, which covers every caller Activity and every pair of AiSdkPlugins. The gaps are at the edges. A plugin configured after AiSdkPlugin still replaces its Activities without an error, and that includes any plugin on the NativeConnection. The new error's remedies also don't fit the two collisions most likely to raise it: StrandsPlugin's fixed invokeModel and invokeModelStreaming, and two AiSdkPlugins built separately with equal options.
Release version. @temporalio/vercel-ai-sdk 1.0.0 is published as Generally Available, and since that tag the plugin has had only metadata and test changes (#116, #259). After this change, a Worker that starts on 1.0.0 can fail Worker.create following a ^1.0.0 upgrade, which makes it a semver-major change. The release tooling doesn't enforce semver, so please state in the PR body that the next tag is 2.0.0. The alternative is to log a warning in 1.x and make it an error in 2.0. Separately, AiSdkPlugin and its options still carry @experimental JSDoc, which contradicts the GA maturity either way.
On the open questions: (1) One policy across the family seems right. Refusing different instances is the safer policy, because last-wins is what produced #160, and #202 already words the Strands remedy this way. (2) A check in SimplePlugin's resolveAppendObjectParameter upstream would catch both orders for every SimplePlugin-based plugin, including third-party ones, which no per-plugin configureWorker check can do. That is worth an sdk-typescript issue but needn't block this. (3) Agreed that it's a separate change. The PR body still says 'Draft while these questions are open', but the PR is not marked as a draft.
Decision: comment rather than approve. Nothing here is a blocking correctness defect, but the release version for a published GA 1.0.0 package and your own open questions are unsettled, and both P2s concern what the new error tells users.
| * `mcpConnectionIdleTimeout`. | ||
| */ | ||
| override configureWorker(options: WorkerOptions): WorkerOptions { | ||
| refuseReplacedActivities('AiSdkPlugin', options.activities, this.options.activities as object); |
There was a problem hiding this comment.
P2 (non-blocking): the check only sees Activities registered before this plugin, so a plugin configured after AiSdkPlugin still silently replaces invokeModel and invokeModelStreaming. That includes every plugin on the NativeConnection, because Worker.create applies the Worker's plugins first and the connection's after them. The ordering comes from options.plugins = (options.plugins ?? []).concat(options.connection?.plugins ?? []) in @temporalio/worker 1.24.0, lib/worker.js:196. I probed this with real Worker.create calls against a dev server, pairing AiSdkPlugin with a SimplePlugin that registers invokeModel the way StrandsPlugin does on main. With AiSdkPlugin on the Worker and Strands on the connection, Worker.create succeeds and the compiled worker.options.activities.get('invokeModel') is Strands' handler. plugins: [ai, strands] on the Worker behaves the same way. With Strands on the Worker and AiSdkPlugin on the connection, you get the new TypeError. So which of two equivalent setups fails depends on list order and on where each plugin is registered. In the setup that passes, the Vercel Workflows' model calls reach Strands' handler, which is the #160 symptom. #202 closes this for Strands and #161 for Google ADK's MCP names, but only once they land, and never for third-party or caller-written plugins. The plugin can close its own side regardless of order. worker.options is public, and worker.options.activities is a Map holding the original function references (the probe above reads it). So AiSdkPlugin can override runWorker(worker, next), check that each name in this.options.activities still maps to its own function or one with the same sources, and throw before calling next(worker) if not. That catches a later replacement before polling starts. Replay Workers skip runWorker, which is fine because they run no Activities. If you'd rather keep this PR to #160's scope, say in the README that only registrations ahead of AiSdkPlugin are checked and that NativeConnection plugins count as after the Worker's own. Then add a test that pins that behaviour, so that a later change to it is deliberate.
There was a problem hiding this comment.
Added the runWorker check in 58c9ca2. runWorker runs after every plugin's configureWorker, so it reads the final worker.options.activities. It throws if a plugin applied after AiSdkPlugin (later in plugins, or on the NativeConnection) replaced one of its Activities with a different one.
Before throwing, it stops the native Worker and lets next run, so the Worker never polls and run()'s own teardown releases it. Without that step, connection.close() in the usual finally block throws "Cannot close connection while Workers hold a reference to it" and hides the TypeError. There's no public way to stop a Worker that hasn't run, so this uses the protected nativeWorker.
New test: runWorker refuses an Activity that a later plugin replaced, before polling. It fails without the check and without the release step. Against a dev server, both of your setups ([ai, strands] on the Worker, and Strands on the connection) now fail worker.run(), and the Workflow's history shows the refused Worker took no task. #202 and #166 are adding the same check, and a follow-up PR adds it to GoogleAdkPlugin, since #161 merged without it. A check in SimplePlugin would still be the better place; I'm drafting an sdk-typescript issue for it.
There was a problem hiding this comment.
Follow-up in 3f769db: the first version of this check awaited before starting the Worker, even when nothing was replaced. Worker.runUntil stops the Worker after its callback only if it is already running, so a callback that threw synchronously left runUntil polling forever, and so did two plugins with the check. runWorker now hands on to super.runWorker in the same tick when the check passes, and a new test covers both cases.
| if (other === undefined || other === fn || sameSources(other, fn)) continue; | ||
| throw new TypeError( | ||
| `Duplicate registration of Activity '${name}': ${plugin} would replace a different Activity with this name. ` + | ||
| `Rename or remove the other registration, or give each ${plugin} on this Worker the same options.` |
There was a problem hiding this comment.
P2 (non-blocking): neither remedy in this message applies to the two collisions most likely to raise it. (a) StrandsPlugin always registers invokeModel and invokeModelStreaming (strands-agents/src/plugin.ts:80-81 on main), and neither plugin can rename them. A Worker with StrandsPlugin ahead of AiSdkPlugin now fails with 'Rename or remove the other registration, or give each AiSdkPlugin on this Worker the same options'. The only real fix is to put one of the two integrations on a separate Worker and task queue, and the message doesn't say so. (b) 'The same options' actually means the same objects. I probed two cases where the options are equal but the objects are not. Two plugins each built with modelProvider: createOpenAI({ apiKey: 'k' }) are refused on invokeModel. Two plugins with the same provider and an MCP factory written inline in each ({ files: async () => createMCPClient(...) }) are refused on files-listTools. Both get this message, even though the caller did pass equal options. Constructing the plugin twice like this is the realistic way to end up with one instance on the NativeConnection and another on the Worker. Suggested wording: Rename or remove your Activity with this name, run the other integration on a separate Worker and task queue, or register a single ${plugin} instance (two instances match only when they share the same modelProvider and MCP client factory objects). #202's README already says 'run the other integration on its own worker', and matching wording across the family would help.
There was a problem hiding this comment.
Reworded in 58c9ca2, using your suggestion without the parenthesis: ... Rename or remove the other Activity, run the other integration on its own Worker and task queue, or use one AiSdkPlugin instance on this Worker. #202 and #166 use the same text, and so does the new runWorker error. The identity rule (the same modelProvider object, and for each MCP server the same factory and idle timeout) is in the README instead, to keep the message short.
| The plugin registers Activities named `invokeModel`, `invokeModelStreaming` and | ||
| `invokeEmbeddingModel`, plus `<server>-listTools` and `<server>-callTool` for | ||
| each MCP server. If the Worker already has a different Activity with one of | ||
| these names (your own Activity, another integration's MCP Activity, or an |
There was a problem hiding this comment.
P3: the README gives 'another integration's MCP Activity' as the only cross-integration case and 'Rename the other Activity or MCP server' as the fix, but the collision that is certain to happen is on the model Activities. @temporalio/strands-agents' StrandsPlugin also registers invokeModel and invokeModelStreaming, so once this PR and #202 land, the two plugins cannot share a Worker in either order. Name StrandsPlugin here and give the fix: separate Workers and task queues. Also say that 'the same options' means the same modelProvider and factory objects, or one shared instance. Finally, state which registrations the check sees (see the plugin.ts comment).
There was a problem hiding this comment.
Done in 58c9ca2. The README now names StrandsPlugin and says to run the two integrations on separate Workers and task queues. It says two instances count as the same only when they share the modelProvider object and, for each MCP server, the factory and idle timeout, and that two equal object literals are refused. It also says which registrations fail at Worker.create (registered before the plugin) and which at worker.run() (applied after it).
The configureWorker check sees only Activities registered before AiSdkPlugin, so a plugin applied after it (later in plugins, or on the NativeConnection) still replaced invokeModel and the other plugin Activities without an error. AiSdkPlugin.runWorker now checks the Worker's final Activities and throws a TypeError if one of its Activities was replaced by a different one. Before throwing, it stops the native Worker and lets run() release it, so the Worker never polls and connection.close() still works. The error now names the fixes that apply: rename or remove the other Activity, run the other integration on its own Worker and task queue, or use one AiSdkPlugin instance on this Worker. The README names StrandsPlugin, says when two instances count as the same, and says which registrations fail at Worker.create and which at worker.run().
|
Thanks for the review. 58c9ca2 covers the three threads; I replied in each. On the summary:
|
campoy
changed the title
Worker.runUntil stops the Worker after its callback only if the Worker is already running. Awaiting the check first meant a callback that throws synchronously, or two plugins with the check, left runUntil polling forever.
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Re-review at head 3f769db (three commits on 5e91c26; it merges cleanly with current main 88745dc). On a worktree at the head with pnpm 10.27.0: make lint clean, make test 36 passed and 1 skipped. On the head merged into current main: make lint clean, make test 70 passed and 1 skipped. Against the base source, the new configureWorker test fails at its first refusal and the later-plugin test times out because run() starts polling. The runUntil test passes on the base too, which is expected: it guards this PR's own runWorker override, not #160.
I verified the run-time check with probes against the built package and real Workers on a local dev server (CLI 1.9.1). A refused Worker with Workflows and Activities, on a queue that already held a scheduled Workflow Task, rejected worker.run() with the TypeError in 6 ms, ended STOPPED, and connection.close() succeeded. DescribeTaskQueue then listed no pollers of either type, while a control Worker on another queue shows up in both. The history stayed at WorkflowExecutionStarted, WorkflowTaskScheduled. The same refusal holds for a replacing plugin on the NativeConnection, for a later plugin that replaces files-listTools, and for two instances built from one options object followed by a replacing plugin (two refusals in one chain, still STOPPED and closable). Under runUntil with a callback that has not finished, the TypeError arrives as cause.workerError of a CombinedWorkerRunError, which is the SDK's own runUntil behaviour when the Worker stops first. Checked and fine: interceptor-only plugins such as OpenTelemetryPlugin register no Activities and do not trip the check, and replay Workers skip plugin runWorker while configureReplayWorker carries no Activities. Replay: this changes Worker configuration only. The Workflow-side modules are byte-identical to the base, and Activity names, inputs and results are unchanged, so a 1.0.0 history replays as before and no version gate is needed. Not run: the real StrandsPlugin and GoogleAdkPlugin classes next to this head. I read their side from main (#202, #161) and simulated it with SimplePlugin stand-ins.
Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.
Previous rounds. Addressed: a plugin applied after AiSdkPlugin, including one on the NativeConnection, is now refused by the runWorker check before the Worker polls (verified above). The error's remedies now name a separate Worker and task queue and a single instance, matching #202. The README names StrandsPlugin, gives the identity rule, and says which registrations fail at Worker.create and which at worker.run(). The PR body names 2.0.0 as the next tag and settles the three open questions, and the draft line is gone. The same-tick handoff from 3f769db holds: when the check passes, runWorker reaches runInternal synchronously through super.runWorker and the runContext. Not addressed, and not re-raised because it predates this PR: AiSdkPlugin and AiSdkPluginOptions still carry @experimental JSDoc, which contradicts the GA maturity.
Parity. This now matches StrandsPlugin on main: the same two checks with the same remedy. GoogleAdkPlugin on main still has only the configureWorker check, with the older remedy; #339 brings it in line, and #166 covers OpenAI Agents. The Python SDK's Worker already rejects duplicate Activity names, so no Python change is needed.
Decision: approve. Both orders are now refused as documented, CI is green, nothing from the last round is still blocking, and the two items below are non-blocking documentation fixes.
PR body (P3, non-blocking): the real-Worker bullet says that, with AiSdkPlugin before GoogleAdkPlugin 0.1.0 sharing an MCP server name, 'The other order still starts; #161 covers it.' At this head that order no longer runs. With a SimplePlugin registering files-listTools and files-callTool after AiSdkPlugin({ modelProvider, mcpClientFactories: { files } }), Worker.create succeeds and worker.run() rejects with Duplicate registration of Activity 'files-listTools': a plugin applied after AiSdkPlugin replaced its Activity with this name. GoogleAdkPlugin from main refuses the same setup earlier, in its own configureWorker (#161). The bullet predates 58c9ca2.
| run the two integrations on separate Workers and task queues. Two `AiSdkPlugin` | ||
| instances count as the same only when they share the `modelProvider` object | ||
| and, for each MCP server name, the same factory function and idle timeout, as | ||
| when both are built from one options object. Two equal object literals are |
There was a problem hiding this comment.
P3 (non-blocking): 'Two equal object literals are refused' says the opposite of what the code does in the common case. Equal option literals pass whenever they reference the same provider and factory objects. The PR's own test accepts new AiSdkPlugin({ modelProvider: provider, mcpClientFactories: { files } }) written out twice (test-ai-sdk.ts:994, 'the same options twice'). I chained configureWorker the way Worker.create does. Two { modelProvider: openai } literals (the @ai-sdk/openai singleton) are accepted, and so are two { modelProvider: provider, mcpClientFactories: { files } } literals. What is refused is a provider or factory created once per plugin: createOpenAI({ apiKey: 'k' }) in each literal fails on invokeModel, and an inline files: async () => ... in each fails on files-listTools. Also, 'for each MCP server name' reads as if both instances must register the same servers. In fact two instances with the same provider and disjoint servers ({ a } and { b }) are accepted, and the Worker gets both servers' Activities. Suggested: 'Two AiSdkPlugin instances count as the same only when they share the modelProvider object and, for each MCP server name both register, the same factory function and idle timeout. A provider or factory created separately for each instance is refused even when its settings are equal, for example createOpenAI({ apiKey }) called once per plugin, or an async () => ... factory written inline in each options object.' The sentence matches #202's README. There it holds for the models map, because StrandsPlugin compares that object itself.
What was changed
AiSdkPluginnow refuses a different Activity with one of its names.configureWorkerthrows when one of the plugin's Activities would replace an Activity registered before it, andrunWorkerthrows when a plugin applied after it replaced one of its Activities:super.configureWorker, the plugin compares its Activities with the Worker's, read the way the Worker reads them (own enumerable keys with function values).TypeErrorand the wording follow the Worker's ownDuplicate registration of nexus servicecheck.activity-sources.tsrecords, for each Activity fromcreateActivities, the operation it was built for and what it was built from: the provider for the model Activities, the MCP client factory and idle timeout for the MCP Activities. Activities with the same operation and sources may replace each other. So the same instance twice, two instances with the same options, andcreateActivities(provider)next to anAiSdkPluginwith the same provider still pass. Another operation under a plugin name, such ascreateActivities(provider).invokeEmbeddingModelregistered asinvokeModel, is refused.runWorkerruns after every plugin'sconfigureWorker, so it sees what a plugin later inplugins, or any plugin on theNativeConnection, did to the finalworker.options.activities. Before it throws, it stops the native Worker and letsrun()release it, so the Worker never polls andconnection.close()still works. There is no public way to stop a Worker that has not run, so this uses the Worker's protectednativeWorker.StrandsPlugin, which registersinvokeModelandinvokeModelStreamingtoo.Breaking change: Workers that start today with a collision now fail: in
Worker.createfor a caller Activity with a plugin name, twoAiSdkPlugins with different options, or another integration's Activity registered first; inworker.run()for a plugin applied afterAiSdkPluginthat replaces one of its Activities. Rename or remove the other Activity, or run the other integration on its own Worker and task queue. The commit subject says so, because release notes come from it.Release:
@temporalio/vercel-ai-sdk1.0.0 is published as Generally Available, so this is a semver-major change. The next tag after this lands should be 2.0.0.Rollback: this changes Worker configuration, not Workflow code. Activity names, inputs and results don't change, so recorded histories replay unchanged with this change and with 1.0.0.
Why?
See #160. The SDK's plugin merge keeps the last Activity of each name, so a caller Activity, another
AiSdkPlugin's handlers, or another integration's MCP Activity was replaced without a warning, and Workflows reached the wrong handler.Checklist
Closes [Bug] vercel-ai-sdk: AiSdkPlugin silently replaces another Activity with the same name #160
How was this tested:
configureWorker refuses to replace a different Activity with the same nameinsrc/__tests__/test-ai-sdk.ts. Onmainit fails at its first refusal (configureWorkerreturns instead of throwing). With this change it passes.runWorker refuses an Activity that a later plugin replaced, before polling: a real Worker withAiSdkPluginand then a plugin registeringinvokeModel. It checks theTypeError, that the Worker endsSTOPPED, and thatconnection.close()succeeds. It fails without therunWorkercheck (run()starts polling) and without the release step (the Worker staysINITIALIZED).runWorker starts the Worker at once, so runUntil stops it after its callback: when nothing was replaced,runWorkerstarts the Worker without yielding, becauseWorker.runUntilstops the Worker after its callback only if it is already running. A callback that throws synchronously rejects and leaves the WorkerSTOPPED, and so does an immediately resolved callback with two plugins built from one options object. With anawaitbefore starting the Worker, the test times out.make lintpasses.make test: 33 passed onmain, 36 with this change (1 skipped in source).check_conventions.pypasses.run():plugins: [ai, later], andaion the Worker withlateron theNativeConnection, both failworker.run()with theTypeError. The Workflow's history stays atWorkflowExecutionStarted, WorkflowTaskScheduled, so the refused Worker took no task, and a Worker without the later plugin then completes it. The same instance on theNativeConnectionand the Worker, with a later plugin on the connection, is refused the same way, andconnection.close()succeeds.main. With this change, both collisions fail with theTypeError.@temporalio/worker1.24.0, Temporal CLI 1.9.1), this change next to@temporalio/google-adk0.1.0: a callerinvokeModel, and an embedding Activity registered asinvokeModel, next toAiSdkPluginfailWorker.create.AiSdkPluginalone andcreateActivities(provider)next to it start and answer.GoogleAdkPluginthenAiSdkPluginwith one MCP server name now fails. The other order still starts; Fail Worker creation when GoogleAdkPlugin would replace another Activity (breaking) #161 covers it.AiSdkPlugin(the same instance, two with the same options, or one on theNativeConnectionand one on the Worker) passes the new check. WithworkflowsPath,Worker.createstill fails in the bundler, as on 1.0.0: each instance prependspreload-polyfills.jsto the webpack entry and webpack refuses the duplicate. On a Worker without Workflows it starts and answers.CI covers the other Node versions and platforms.
Any docs updates needed?
The README paragraph, included here. The docs.temporal.io guide doesn't list the plugin's Activity names; a line can follow the release.
Related: #161 added the
configureWorkercheck toGoogleAdkPlugin(#159), with the earlier message. A follow-up PR brings it the same message andrunWorkercheck. #202 (StrandsPlugin) and #166 (OpenAIAgentsPlugin) are adopting the same message and check.Earlier open questions, now settled:
SimplePlugin(sdk-typescript) would cover every plugin in both orders. That needs an SDK change and release, so this PR keeps its own checks.AiSdkPluginwithworkflowsPathstill fails in the bundler, as on 1.0.0. That is a separate change toconfigureBundler.