[vitest-pool-workers] Treat webSocket* as optional Durable Object handlers#14763
Conversation
…dlers The pool wraps each Durable Object class and installs a prototype method for every key in `DURABLE_OBJECT_KEYS` before user code is loaded. `workerd` therefore always sees a handler and always dispatches, and the wrapper threw a TypeError when the wrapped class turned out not to define one. That is wrong for `webSocketMessage()`, `webSocketClose()` and `webSocketError()`, which deployed Workers treat as optional and silently ignore when absent. A hibernatable Durable Object defining only `webSocketMessage()` logged an uncaught TypeError on every close. No-op these three keys instead of throwing. `alarm()` deliberately keeps the old behaviour: `workerd` rejects `setAlarm()` up front on a class with no `alarm()` handler, so a missing one is a genuine error. Verified the production semantics against raw `workerd` (via Miniflare, no pool wrapper): a hibernatable Durable Object with no `webSocket*` handlers accepts, receives and closes a socket without error, while `setAlarm()` on a class with no `alarm()` handler throws "Your Durable Object class must have an alarm() handler in order to call setAlarm()". The `Counter` fixture's empty `webSocketClose()`/`webSocketError()` stubs were a workaround for this and are now removed.
🦋 Changeset detectedLatest commit: 7449959 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
@cloudflare/autoconfig
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
edmundhung
left a comment
edmundhung
left a comment
There was a problem hiding this comment.
Thanks for the PR! The fix looks good to me.
Could we move the test into packages/vitest-pool-workers/test/bindings.test.ts? The examples fixture is user-facing, and I believe this regression test is more about the behavior of our internal wrapper.
You can then use the vitestRun() helper to assert that stderr does not contain the TypeError.
Addresses review feedback: the examples fixture is user-facing, and this is a test of the internal Durable Object wrapper rather than a recipe for users. Reworks it as a seeded `vitestRun()` case in `test/bindings.test.ts` that asserts the child run exits 0 and that its output never mentions "does not define". This is a genuine red-to-green test: without the fix the run reports `OptionalWebSocketHandlers ... does not define a `webSocketClose()` method`. Making it deterministic needed one extra step. The uncaught exception is logged asynchronously, so a test that merely closes the socket can finish before the dispatch happens. The seeded test now polls a `socketCount()` RPC until the runtime has actually processed the close. Reverts the fixture additions, keeping only the removal of `Counter`'s empty `webSocketClose()`/`webSocketError()` stubs, which the fix makes unnecessary.
|
Thanks — done, and this turned out much better than what I had. Pushed as a separate commit rather than a force-push. The test now lives in Your suggestion also fixed the caveat I'd flagged in the description — it's now a genuine red-to-green regression test. Without the fix the seeded run reports: Two notes on details I hit getting there, in case they're useful:
I've kept one thing from the fixture: the removal of Verified locally: |
workers-devprod
left a comment
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
|
Thanks for the approval! This is currently blocked on Why I think it's unrelated:
One thing I can't fully rule out from the logs: whether adding another seeded |
3 participants
Fixes #13639.
createDurableObjectWrapper()installs a prototype method for every key inDURABLE_OBJECT_KEYS(connect,fetch,alarm,webSocketMessage,webSocketClose,webSocketError) before user code is loaded.workerdprobes the prototype to decide whether a Durable Object implements a handler, so it always finds one and always dispatches. The wrapper then looks the method up on the real user instance and, if it isn't there, throws:Deployed Workers treat
webSocketMessage(),webSocketClose()andwebSocketError()as optional — a class that omits them just has those events ignored. So a hibernatable Durable Object defining onlywebSocketMessage()(exactly the reporter's case) works when deployed but logs an uncaughtTypeErroron every close.The change
Those three keys now no-op instead of throwing. Because the prototype is built before user code loads, the wrapper can't simply leave them undefined to inherit the runtime's behaviour — the no-op has to be reproduced explicitly.
alarm()is deliberately not in the optional set, and still reports a missing handler.Verifying the production semantics
Rather than infer this from the docs, I checked both halves against raw
workerd— driving Miniflare directly, with no pool wrapper in the picture:webSocket*handlers accepts a socket, receives a message and is closed with no error at all — confirming the no-op.setAlarm()on a class with noalarm()handler throwsYour Durable Object class must have an alarm() handler in order to call setAlarm()— confirmingalarmmust keep throwing. A missingalarm()can't even be reached, since the runtime blocks scheduling first.That pins the boundary of the fix rather than leaving it to judgement.
Testing
Regression test in
packages/vitest-pool-workers/test/bindings.test.ts: a seededvitestRun()case with a Durable Object defining onlywebSocketMessage(), asserting the child run exits 0 and its output never mentionsdoes not define.This is red-to-green — without the fix the seeded run reports:
Two details worth noting:
socketCount()RPC until the runtime has actually processed the close. The uncaught exception is logged asynchronously, so without that synchronisation the run can finish before the dispatch happens and passes either way.workerd, not Vitest's error reporting), so the test asserts on both streams.Also removes the empty
webSocketClose() {}/webSocketError() {}stubs from theCounterexample fixture. These were added in #14061 and only ever existed to satisfy this throw; dropping them means the user-facing example no longer demonstrates an unnecessary workaround, and puts the existing hibernation-ordering test on the same path as a real user's class.Verified locally:
bindings.test.ts3/3 with the fix and failing on the new case without it;durable-objectsfixture 9/9 with no uncaught exceptions;oxlint --type-aware,tscandoxfmtclean.webSocket*handlers; no public API or documented behaviour changes.A picture of a cute animal (not mandatory, but encouraged)
🦦