Repository navigation
Breaking: fail Worker creation when a StrandsPlugin Activity would replace another Activity - #202
Conversation
|
Updated in 5520af7: a second |
…place another Activity StrandsPlugin merged its Activities over the Worker's `activities` option. A caller Activity named `invokeModel`, `invokeModelStreaming`, `<server>-listTools` or `<server>-callTool` was dropped with no error, and Workflows calling it ran the plugin's handler. configureWorker now throws a TypeError for such a name. Activities that a StrandsPlugin built are still replaced, so one instance on both the NativeConnection and the Worker still starts. Breaking change: Worker.create now fails when another Activity on the Worker has the name of a Strands plugin Activity. Rename or remove that Activity. Activity names, inputs and History do not change.
… Worker The last StrandsPlugin on a Worker replaced the Activities of an earlier one. With `models` on one instance and only `mcpClients` on another, the second instance's default Bedrock model served agents that named no model, with no error. Each plugin Activity now records what it was built from: its name, the `models` object (none for the default), or the MCP factory and idle timeout. configureWorker refuses to replace an Activity built from something else, as GoogleAdkPlugin does. The same instance on the NativeConnection and the Worker, and instances built from one options object, still start. Breaking change: Worker.create now also fails for two StrandsPlugins with different options whose Activity names overlap. Put every model and MCP client in one StrandsPlugin, or build both from one options object.
5520af7 to
d2be784
Compare
DABH
left a comment
There was a problem hiding this comment.
Review at head d2be784 (two commits over 1e8ae93). On a worktree at the head with pnpm 10.27.0 on PATH: make lint clean, make test 71 of 71 including the new test. Findings were verified with probes against the compiled head: configureWorker chains for each refusal and allowance the body lists, and real Worker.create calls against a local dev server through TestWorkflowEnvironment.createLocal with plugins on its NativeConnection. Checked and sound: the own-enumerable test matches exactly what SimplePlugin's { ...existing, ...param } merge replaces; the same instance on the NativeConnection and the Worker, one options object twice, two default instances, a default plus an MCP-only instance, and an omitted idle timeout next to '5 minutes' all start; Replayers are unaffected because configureReplayWorker merges no Activities, so the rollback claim holds; strandsGetModelConfig is covered, and #285 adds its two stateful Activities to the same object literal ahead of the sources loop (plugin.ts:99), so they are covered after a rebase with no code change; #200 adds no Activities and only touches the same @temporalio/worker import line. Both outcomes the body flags are reasonable: model factories are opaque closures, so identity is the only equality that cannot bring back a silent replacement, and before this change an AiSdkPlugin next to a StrandsPlugin silently ran one integration's invokeModel for the other's Workflows with the wrong input shape, so failing at start-up is strictly better. Not verified: the real AiSdkPlugin (not installed in this plugin's environment; a stand-in SimplePlugin registering the same two names was used), and the macOS, Windows and Node 22 cells beyond CI, which is green.
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 in every case probed; all three items are places where the README, the JSDoc or the error message describe the rule less precisely than the code applies it. Approving because no P1 or P2 was found, CI is green, and the three P3 items below are non-blocking.
Outside this PR, not blocking and possibly worth an issue: the check is per Worker, but the MCP connection cache is process-wide and keyed by server name only (temporal-mcp-client.ts:216, read in acquireConnection at 238-264). Two Workers in one process with different factories for the same server name share whichever connection opened first (probe: the second plugin's s-listTools returned the first factory's tools; same on main), and either Worker's shutdown evicts it (plugin.ts:122-124).
|
|
||
| The read is a new activity, so a run whose agent ran before you deploy this version fails to replay on it with a nondeterminism error. Let those runs finish first, or use Worker Versioning. | ||
|
|
||
| The plugin's activities are named `invokeModel`, `invokeModelStreaming`, and `{name}-listTools` and `{name}-callTool` for each MCP client. If the worker already has a different activity with one of these names (your own activity, another integration's activity, or one from a second `StrandsPlugin` with different options), creating the worker fails with `Duplicate registration of Activity '<name>'`. Rename your activity, run the other integration on its own worker, or put every model and MCP client in one `StrandsPlugin`. The same instance on the `NativeConnection` and the worker, or two instances built from the same options object, still work. |
There was a problem hiding this comment.
P3 (non-blocking): the list of the plugin's activity names leaves out strandsGetModelConfig, which the check also refuses. A caller activities: { strandsGetModelConfig: async () => 1 } next to new StrandsPlugin({ models: { stub } }) throws Duplicate registration of Activity 'strandsGetModelConfig' (probe against the compiled head), and the paragraph two above this one names that activity. The PR body's breaking-change list has the same gap. #285 adds strandsInvokeStatefulModel and strandsInvokeStatefulModelStreaming to the same object literal ahead of the sources loop, so the check covers them with no code change, but whichever PR lands second should add them here. Fix: "The plugin's activities are named invokeModel, invokeModelStreaming, strandsGetModelConfig, and {name}-listTools and {name}-callTool for each MCP client", and add the name to the body's list.
There was a problem hiding this comment.
Thanks, fixed in 14315e6. The README list now names strandsGetModelConfig, and also strandsInvokeStatefulModel and strandsInvokeStatefulModelStreaming, since #285 landed first (merged into this branch in 008e7a3). The check covers them with no code change. I'm adding them to the breaking-change list in the PR description too.
| if (typeof other === 'function' && other !== fn && !sameSources(other, fn)) { | ||
| throw new TypeError( | ||
| `Duplicate registration of Activity '${name}': StrandsPlugin would replace a different Activity ` + | ||
| 'with this name. Rename or remove the other registration, or give each StrandsPlugin on this Worker ' + |
There was a problem hiding this comment.
P3 (non-blocking): the message tells the user to "give each StrandsPlugin on this Worker the same options", but equal options are refused unless they share the models object and the MCP factory functions, so someone who hits the identity rule is told to do what they already did. new StrandsPlugin({ models: { stub } }) twice, with the same stub function, throws this message for invokeModel. A realistic way to get there: TestWorkflowEnvironment.createLocal({ plugins: [new StrandsPlugin({ models: { test: () => stub } })] }) passes the plugins to env.nativeConnection as well as the client, and Worker.create({ connection: env.nativeConnection, plugins: [new StrandsPlugin({ models: { test: () => stub } })] }) then fails with this TypeError against a local dev server; the same two calls with one shared models constant start. README line 98 says "a second StrandsPlugin with different options" and has the same gap. Identity is a sound rule; the wording should state it. Suggested message tail: ...or register one StrandsPlugin instance, or build each from the same models object and MCP client factories. README: "Two instances count as the same only when they share the models object (or both omit it) and, for each MCP server name they share, the same factory function and idle timeout. Two equal object literals are refused." The text is shared with #161 and #162, so change it in all three or in none. Optional: compare models per key (same keys, each factory ===, same derived default) instead of by object; ModelActivity only reads the factories and the default name (model-activity.ts:84-108), so that would accept { models: { stub } } twice and still refuse inline arrow factories.
There was a problem hiding this comment.
Agreed, changed in 14315e6. The message now ends: "Rename or remove the other Activity, run the other integration on its own Worker and task queue, or use one StrandsPlugin instance on this Worker." #162 and #166 use the same remedies. The README now states the identity rule: "Two instances count as the same only when they share the models object (or both omit it) and, for each MCP server name they share, the same factory function and idle timeout, as when both are built from one options object. Two equal object literals are refused."
I kept the comparison by models object rather than per key. Per key only helps when the factories are shared but the models literal isn't. Your TestWorkflowEnvironment example uses inline factories, which a per-key compare would still refuse.
#161 was merged with the old text before this round, so GoogleAdkPlugin would need a small follow-up to match.
| * Adds the model and MCP Activities to the Worker. | ||
| * | ||
| * @throws TypeError if the Worker already has a different Activity with one of these names: a | ||
| * caller Activity, another integration's Activity, or one from a `StrandsPlugin` with different |
There was a problem hiding this comment.
P3 (non-blocking): "another integration's Activity" is refused only when that integration is registered before StrandsPlugin; a later plugin still replaces these Activities silently, and neither this JSDoc nor README line 98 says so. Worker.create applies the Worker's plugins in order and then connection.plugins (@temporalio/worker 1.24.0 worker.js:196-201). With a stand-in SimplePlugin registering invokeModel and invokeModelStreaming (the names AiSdkPlugin registers, vercel-ai-sdk/src/activities.ts:240,255): [new StrandsPlugin(), other] starts and invokeModel is the other plugin's function; with other on TestWorkflowEnvironment's NativeConnection and StrandsPlugin on the Worker, a real Worker.create starts and worker.options.activities.get('invokeModel') is the other function; [other, new StrandsPlugin()] is refused. The body says this ("registered first"; #162 closes the AiSdkPlugin case in both orders), but someone reading the README would not know it, and until #162 lands plugins: [new StrandsPlugin(), new AiSdkPlugin()] starts. Fix: add to README line 98 and to this JSDoc: "The check sees only activities registered before StrandsPlugin: the worker's own activities and plugins earlier in the worker's plugins. A plugin later in that list, or any plugin on the NativeConnection (applied after the worker's plugins), can still replace these activities unless it checks too."
There was a problem hiding this comment.
Thanks, done in 373a9a6, and it goes one step further than docs. StrandsPlugin now also overrides runWorker, which runs after every plugin's configureWorker. If a plugin applied after it replaced one of its Activities with a different function, it stops the Worker before it polls, lets it release the connection, and throws Duplicate registration of Activity '<name>': a plugin applied after StrandsPlugin replaced its Activity with this name., followed by the same remedies. This is the same check #162 added in 58c9ca2, so a later plugin, including one on the NativeConnection, no longer slips through in either PR.
The README now says: "If it is registered before StrandsPlugin (in the worker's activities or by an earlier plugin), creating the worker fails. If a plugin applied after StrandsPlugin replaces one, worker.run() fails before the worker polls. Worker.create applies the worker's plugins in order, then the NativeConnection's." The runWorker JSDoc says the same.
The new test, runWorker refuses an Activity that a later plugin replaced, before polling, runs a real Worker with [new StrandsPlugin(...), laterPlugin] and checks the TypeError, that the Worker ends STOPPED, and that connection.close() succeeds. Without the check, run() starts polling and never returns. Without the release, the state stays INITIALIZED.
A check in SimplePlugin's merge would still be the general fix for every plugin, but that needs an SDK change.
…duplicate error The README now names strandsGetModelConfig and the two stateful model Activities, and says when two StrandsPlugin instances count as the same. The error's remedies no longer say "the same options", since instances are compared by object identity.
… replaced configureWorker sees only Activities registered before StrandsPlugin, so a plugin applied after it could still replace one silently. runWorker runs after every plugin's configureWorker: if one of the plugin's Activities was replaced by a different function, it stops the Worker before it polls, lets it release the connection, and throws.
|
Thanks for the review, @DABH. All three P3s are addressed, in 14315e6 (README names, error remedies and the identity rule) and 373a9a6 (a On the MCP connection cache: agreed, it's outside this PR and also on |
What was changed
StrandsPlugin.configureWorkernow throws when one of the plugin's Activities would replace a different Activity with the same name:StrandsPluginbuilds records what it was built from, in a module-levelWeakMap: its name, then themodelsobject and default model name (nothing for the defaultBedrockModel), or the MCP client factory and effective idle timeout. Beforesuper.configureWorker, an own enumerable entry of the Worker'sactivitieswith one of the plugin's names is refused unless it is the same function or was built from the same things. The hook and check follow Fail Worker creation when GoogleAdkPlugin would replace another Activity (breaking) #161 forGoogleAdkPlugin; the error text is shared with Refuse a different Activity under an AiSdkPlugin Activity name (breaking) #162 and Breaking: fail Worker creation when an OpenAI Agents plugin Activity would replace another Activity #166.StrandsPluginwith different options are refused. Setups that are really one plugin still start: the same instance on theNativeConnectionand the Worker (whereconfigureWorkerruns twice), two instances built from one options object, and two defaultStrandsPlugin()s. An omittedmcpConnectionIdleTimeoutcounts as its five-minute default. Two instances with different MCP server names keep both servers.StrandsPlugin.runWorker, which runs after every plugin'sconfigureWorker, refuses an Activity that a plugin applied afterStrandsPluginreplaced with a different function: it stops the Worker before it polls, lets it release the connection, and throwsDuplicate registration of Activity '<name>': a plugin applied after StrandsPlugin replaced its Activity with this name.with the same remedies. Same check as Refuse a different Activity under an AiSdkPlugin Activity name (breaking) #162.Worker.createand what failsworker.run(), how to fix it, and when two instances count as the same.Breaking change: Workers that start today with a collision now fail in
Worker.create:invokeModel,invokeModelStreaming,strandsGetModelConfig,strandsInvokeStatefulModel,strandsInvokeStatefulModelStreaming,<server>-listToolsor<server>-callTool;AiSdkPlugin'sinvokeModel: registered first, it failsWorker.create; applied afterStrandsPlugin(later in the Worker'splugins, or on theNativeConnection), it failsworker.run()before the Worker polls;StrandsPlugins with different options whose Activity names overlap: for example one withmodelsand one with onlymcpClients(both registerinvokeModel), or two factories or idle timeouts for one MCP server name.modelsobjects are compared by identity, so two instances with equal but separatemodelsobjects are refused too.Rename or remove the other Activity, run the other integration on its own Worker and task queue, put every model and MCP client in one
StrandsPlugin, or build both from one options object. Workers without a clash behave as before. The title starts with "Breaking:" because release notes come from commit subjects.Rollback: this changes Worker configuration, not Workflow code. Activity names, inputs, results and History don't change, so recorded histories replay unchanged with this change and without it.
Why?
See #201. The SDK's
SimplePluginmerges plugin Activities with{ ...existing, ...param }, so a caller Activity with one of the plugin's names was replaced without a warning, and Workflows calling it ran the plugin's handler. The same merge let a secondStrandsPluginreplace the first one's Activities: withmodelson one instance and onlymcpClientson a later one, agents that named no model got the second instance's default Bedrock model.Checklist
Closes [Bug] strands-agents (TypeScript): a plugin Activity silently replaces a caller Activity with the same name #201
How was this tested:
StrandsPlugin refuses to replace a different Activity with the same nameinsrc/__tests__/test-strands.ts; no existing test covers the plugin's Activity registration. Withplugin.tsfrommainit fails at its firstt.throws(configureWorkerreturns options with the plugin'sinvokeModel). With this change it passes. It covers the refusals above (a caller Activity, a models plugin then an MCP-only plugin, anothermodelsobject, another factory or idle timeout for one MCP server) and the setups that still start (the same instance twice, one options object twice, two default instances, an omitted idle timeout next to'5 minutes', different MCP server names).runWorker refuses an Activity that a later plugin replaced, before polling: a real Worker withStrandsPluginand then a plugin registeringinvokeModelfailsrun()with theTypeError, endsSTOPPED, and itsconnection.close()succeeds. Without the check,run()polls and never returns; without the release, the state staysINITIALIZED._typescript-plugin.yml:make sync,make lint,make test(87 tests pass, after mergingmainat34fb981),make build,check_npm_package.pyand the clean-consumer smoke install all pass, andscripts/ci/check_conventions.pypasses. Prettier reports the changed files clean.mainatc664a34. With this change, both names fail with theTypeErrorand it exits 0.main, a models plugin then an MCP-only plugin, and two factories for one MCP server, both start with the last instance's Activities. With this change both are refused, while the same instance twice, one options object twice and two default instances start.@temporalio/*1.24.0): a callerinvokeModelnext to the plugin, and a models plugin with an MCP-only plugin in either order, failWorker.create, and no handler runs. Without the plugin the caller's Workflow gets its own handler's result, and the models plugin alone answers with its model.CI covers the other Node versions and platforms.
Any docs updates needed?
The README paragraph, included here.
Related: #159/#161, #160/#162 and #165/#166 report and fix the same bug in the other TypeScript plugins. These PRs don't depend on each other. With this PR and #162,
AiSdkPluginandStrandsPluginon one Worker fail to start in either order, since both registerinvokeModelandinvokeModelStreaming.Notes:
StrandsPluginwith different options, as Fail Worker creation when GoogleAdkPlugin would replace another Activity (breaking) #161 does forGoogleAdkPlugin. Breaking: fail Worker creation when an OpenAI Agents plugin Activity would replace another Activity #166 still keeps last-wins forOpenAIAgentsPlugin.activitiesobject, before the sources are recorded, so the check covers them with no change.Draft while the breaking change is reviewed.