fix: deliver the actual error to onError(connection, error) when a handler throws#1717
fix: deliver the actual error to onError(connection, error) when a handler throws#1717mattzcarey wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 4788db5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 Pre-existing issue: throw this.onError(e) in _tryCatch is a no-op throw
At packages/agents/src/index.ts:3125, _tryCatch does throw this.onError(e). Since the base onError always throws internally (line 3225), the outer throw is dead code — the exception from onError propagates before it returns. However, if a subclass overrides onError to not throw (which the return type void | Promise<void> allows), then throw this.onError(e) would throw undefined (sync case) or throw a Promise object (async case). This is a pre-existing design quirk unrelated to this PR, but worth noting since the onError contract is being refined here.
(Refers to lines 3121-3127)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed it's a pre-existing quirk and intentionally out of scope here: _tryCatch's throw this.onError(e) is dead code while the base onError throws, and a non-throwing override would make it throw undefined/a Promise. This PR keeps the base contract (always throws) unchanged. Tightening _tryCatch (e.g. await this.onError(e); throw e;) would change observable behavior for subclasses that deliberately swallow errors in an override, so it deserves its own change if maintainers want it.
agents
@cloudflare/ai-chat
@cloudflare/codemode
create-think
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
…ssage handler throws When .sql threw while handling a websocket message, _tryCatch reported it via a one-argument this.onError(error) call, so user overrides written with the documented two-parameter signature received the error as the connection and undefined as the error (#388). _tryCatch/_tryCatchChat now route the actual caught error through onError(connection, error) using the connection from the agent context, and rethrow the original error instead of onError's return value. The base onError discriminates overloads on arity and passes errors through exactly as received — never synthesized.
mattzcarey
force-pushed
the
fix/388-onerror-undefined
branch
from
cbcc969 to
8a8f370
Compare
mattzcarey
changed the title
…ect onError docs The state persistence hook (onStateChanged/onStateUpdate) error path also delivered errors via a one-argument onError call even when the update came from a connection. Route it through onError(connection, error) like _tryCatch. Fix the docs examples, which taught the bug: agent-class.md discriminated on error truthiness and threw the first argument (the Connection object on connection errors), and http-websockets.md declared onError twice in one class, which is invalid at runtime. Both now show a single implementation handling both call shapes.
…aths Same ALS store, but the public typed helper instead of reaching into the internal agentContext directly.
1 participant
Note
This is a Fable experiment (model-authored PR). Please close if it adds no value.
Fixes #388
Problem
When
.sql(or anything else) throws while an agent is handling a websocket message, the error is caught by_tryCatch, which reported it with a single-argument call:TypeScript overloads don't exist at runtime. A user override written with the documented two-parameter signature —
onError(connection: Connection, error?: unknown), which is exactly what the reporter of #388 had — therefore receives the thrownSqlErroras theconnectionparameter andundefinedas theerrorparameter. The real error hides in the wrong slot.It gets worse:
throw this.onError(e)throws onError's return value. A user override that returns normally turns the failure intothrow undefined, so the original error also vanishes from upstream logs. That's "my agent is silently failing and i have no clue why."(An earlier revision of this PR misdiagnosed this as the runtime delivering an ErrorEvent with no detail, and synthesized a placeholder Error for that case. That masked the symptom instead of delivering the real error — thanks @threepointone for catching it. The synthesized error is gone.)
Fix
Agent._tryCatch(andAIChatAgent._tryCatchChat) now read the connection from the agent context and deliver the actual caught error through the two-argument overload —this.onError(connection, e)— when the failure happened while handling a connection; server-side failures (alarms, schedules, email) still usethis.onError(e). The original error is then rethrown, not onError's return value.onErrorimplementation discriminates its overloads on call arity instead of error truthiness, so a genuine two-argument call with no error detail (partyserver'sonError(connection, e.error)from an ErrorEvent) is no longer misrouted into the server branch — which used to log and throw the Connection object itself. Errors are passed through exactly as received; nothing is synthesized.The public overload signatures are unchanged.
Tests
packages/agents/src/tests/on-error.test.ts(new), with a dedicatedTestOnErrorAgent:this.sql`SELECT * FROM table_that_does_not_exist`; a user-style two-parameteronError(connection, error)override records what it receives. Asserts the error slot carries the actualSqlError("no such table") and the connection slot carries the Connection. Red on main:errorisundefinedand the SqlError arrives asconnection.TestStateAgent:(connection, error)and(error)rethrow the original error;(connection, undefined)passesundefinedthrough — it neither throws the Connection object nor synthesizes a stand-in error.pnpm run checkgreen (format, lint, 102-project typecheck); fullagentssuite 1760 passed; full@cloudflare/ai-chatsuite 659 passed.Notes for maintainers
onStateChanged/onStateUpdatethrowing) is also routed throughonError(connection, error)when the update came from a connection — same bug, same fix, covered by a test.docs/agent-class.md's example discriminated on error truthiness and threw the first argument (i.e. the Connection object on connection errors), anddocs/http-websockets.mddeclaredonErrortwice in one class, which is invalid at runtime. Both now show a single implementation handling both call shapes.await this.onError(e)in queue/schedule/alarm/agent-tool-recovery paths) are genuinely server-side — no connection in context — and are unchanged._tryCatchrethrowing the original error means handler failures now surface as the real exception in runtime logs even when a user override swallows them silently before — previously those paths threwundefined.agents+@cloudflare/ai-chat, both patch).