feat: request/response messaging for room events - #1513
Conversation
Deploying js-sdk-toolchain with
|
| Latest commit: |
bee191b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://096144bd.js-sdk-toolchain.pages.dev |
| Branch Preview URL: | https://feat-room-requests.js-sdk-toolchain.pages.dev |
Test this pull request
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
I found two P1 issues that should be fixed before merging.
Findings
-
P1 — Requests issued before room readiness can still time out before they are ever sent. In
packages/@dcl/sdk/src/network/events/requests.ts:232-234, the sweeper checksdeadlineMsfor every pending entry, including entries markedwaitingForRoom. SinceRoom.send()queues while disconnected, a boot-time request can be rejected beforeonReady(true)has a chance to rebase the deadline. When the room later flushes its queue, the handler may still run even though the caller already observed a timeout and the response will be ignored. Skip sweepingwaitingForRoomentries (or only assign/start the deadline once the request is actually sent), and add a regression test where engine ticks continue past the timeout before the room becomes ready. -
P1 — Server-side
request()withoutoptions.tobroadcasts and accepts the first client response.RequestOptions.tois documented as required for server-initiated requests, butrequest()storesexpectFrom: ''and callsroom.send(..., undefined)whentois omitted (requests.ts:347and:355). On a server room that means the request is broadcast to every client, and the response listener accepts whichever client answers first. This can leak player-specific request payloads and lets an unintended client settle the server's request. Please enforce the runtime contract: server-initiated requests should require a target, and targeted requests should reject responses that lack/mismatchcontext.from.
Additional notes
- Public API impact: this is additive (
registerRequests,createRequests, request types/errors), so I did not find a backward-incompatible consumer impact. - Security review: no hardcoded secrets or injection issues found; the server broadcast/first-response behavior above is the relevant security concern.
- CI: all checks are passing.
Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack
room.send / room.onMessage are fire-and-forget, so a scene that needs an answer
("load my save", "buy this, tell me the new balance") builds the correlation by
hand: a requestId threaded through both payloads, a requester field so each
client can tell whether a broadcast reply was meant for it, a per-handler
reason string for failures, and a retry loop for when no answer arrives.
Adds registerRequests to @dcl/sdk/network:
export const rpc = registerRequests({
loadFarm: { request: Schemas.Map({}), response: FarmStateSchema }
})
rpc.handle('loadFarm', async (_data, ctx) => toPayload(await store.load(ctx.from)))
const farm = await rpc.request('loadFarm', {})
No protocol change: each method derives two ordinary registered events
(@dcl/req:<m>, @dcl/res:<m>) that wrap the declared schemas with a correlation
id, so the event envelope and the wire format are untouched.
The layer owns four things scenes were getting wrong:
- Addressing. The reply goes out with { to: [context.from] }, so one player's
payload never lands in another player's client.
- Errors. Throw RequestError for anything the caller should read and its message
is forwarded verbatim; any other throw becomes internal_error and is logged
server-side, so a crash cannot leak storage keys or stack traces over the wire.
A missing reply rejects with RequestTimeoutError, which is separately
catchable so retry logic can tell it apart from a business rejection.
- Timeouts. Swept from an engine system rather than setTimeout, which the server
runtime does not guarantee. Default 20s, overridable per call.
- Pre-connection sends. room.send queues until the room is ready, so deadlines
are re-based from the moment it connects instead of expiring in the queue.
The response body is always serialized rather than wrapped in Schemas.Optional,
which drops falsy values and would silently turn a legitimate 0 / false / ''
response into a schema default.
registerRequests returns its own typed object instead of adding methods to Room:
Room is generic over the message registry and requests need a different registry
shape, so folding them together would mean either a second type parameter on
Room — changing getRoom<T>() and registerMessages typing for every existing
scene — or losing type safety on one side.
3310e89 to
bee191b
Compare
2 participants
What this adds
Request/response messaging in
@dcl/sdk/network, on top of the existing room events:No protocol change. Each method derives two ordinary registered events (
@dcl/req:<m>,@dcl/res:<m>) that wrap the declared schemas with a correlation id, so the event envelope and wire format are untouched.What it abstracts
room.send/room.onMessageare fire-and-forget, so any scene that needs an answer builds the correlation by hand. Across the shipped scenes that is 286room.sendsites and 207onMessagesites, and four recurring pieces of boilerplate — each of which has produced a real bug.1. Correlation ids, and replies broadcast to everyone
cozy-farm declares the plumbing in the schema (
src/shared/farmMessages.ts:174):then answers without
{ to: … }(src/server/farmServer.ts:44):so every connected client receives every other player's full farm payload and drops it on the floor client-side (
src/services/saveService.ts:503):Same shape at
farmServer.ts:203(playerRegistryLoaded),:224(beautyLeaderboardLoaded),:245(otherFarmLoaded).beautyLeaderboardLoadedalso has a comment noting the personalised rank has to be recomputed per requester even though the list is broadcast.towerofmadness carries an
addressfield for the same purpose and filters against the local player (src/multiplayer.ts:59,:78):With
handle, the reply goes out as{ to: [context.from] }and none of that exists. There is a test asserting the send target is exactly the caller.2. Retry loops standing in for a timeout
My-Dear-Pet re-asks for its own save every 2s for 30s because a lost request is indistinguishable from a slow one (
src/client/setup.ts:107-120):becomes
await rpc.request('loadState', {}), which rejects withRequestTimeoutErrorif nothing answers.3. A hand-rolled error channel per handler
cozy-farm repeats a
reason: 'server_error'string in every catch block (farmServer.ts:186,:248,:284,:323) and, when the load fails, silently sends the player a fresh empty farm (:137):dead-surge declares dedicated rejection messages per request instead (
src/shared/messages.ts:97potionClaimRejected,:244collectibleClaimRejected).Here a handler throws
RequestError('insufficient_funds')and the message is forwarded verbatim; any other throw becomesinternal_errorand is logged server-side, so an unexpected crash cannot leak storage keys, addresses or stack traces over the wire. A missing reply rejects withRequestTimeoutError, which is separately catchable so retry logic can tell a transport failure from a business rejection.4. A payload ceiling nothing enforced
Custom events are not chunked.
LIVEKIT_MAX_SIZE = 12(KB) is applied only to CRDT traffic (network/state.ts:98,106;network/server/index.ts:150,160,298) — agrepfor any size guard onCommsMessage.CUSTOM_EVENTreturns nothing. So an oversized reply is dropped by the transport with no error, and the caller finds out only when its timeout expires. Worse,Room.sendcatches and logs its own failures, so a payload that does not match its schema vanishes the same way.Both are now caught before the send:
checkPayload()encodes the message, rejects a shape the schema cannot serialize (invalid_payload) and one that exceeds the transport limit (payload_too_large). On the handler side, when the reply is the thing that cannot be sent, the failure is sent in its place — so the caller learns why instead of waiting out the clock. Real chunking for large payloads remains a separate piece of work; this makes the ceiling visible rather than silent.5. Timers the server runtime does not have
flagtag documents the constraint (
docs/STORAGE.md:23): "The runtime does not guaranteesetTimeout. Anything time-based on the server (timeouts, retry backoff, debounce) must be driven by an engine system." Deadlines here are swept from an engine system for exactly that reason, sorequest()behaves the same on both sides. Default 20s — a handler may do several ~2s storage round trips — overridable per call.Design notes
The response body is never
Schemas.Optional.IOptional.serializechecksif (value), so a legitimate0/false/''response would serialize as absent and deserialize asundefined. On failure the body is filled fromschema.create()instead, andok: falsetells the caller to ignore it. Regression test included (countCoinsreturning0).Server-initiated requests must name a target, and that is enforced.
rpc.request(m, data, { to: address })is the only valid server form: omittingtowould makeRoom.sendbroadcast the request and its correlation id to every client, any of which could then answer. A server request withouttotherefore throws rather than sending, using a new synchronousRoom.isServer(). The response path additionally fails closed — a reply is only accepted from the peer the request named, so an unset expectation can never be settled from the wire. (Client → server needs no such check:Roomalready drops client-side events from anyone but the authoritative server.)A request queued before the room connects does not burn its budget.
Room.sendqueues while disconnected, so a boot-time request used to be rejected by the sweeper and then executed anyway when the queue flushed — the caller saw a timeout, retried, and the handler ran twice. Queued entries now carry no deadline at all; the clock starts when the room connects and the request actually goes out.Double handlers are flagged, per room. Registering a second live handler for one method means two replies and the caller settling on whichever lands first. The guard is keyed on the
Room(a module-levelWeakMap), not on theRequestsinstance — listeners live on the room, so two instances bound to the same room would each answer, which is exactly the case the warning exists for and which an instance-scoped guard could not see.Why a separate object rather than
room.request/room.handle.Roomis generic over the message registry, and requests need a different registry shape ({ request, response }pairs). Folding both into one class means either a second type parameter onRoom— which changesgetRoom<T>()andregisterMessagestyping for every existing scene — or losing type safety on one side. A separate typed object keeps both fully typed and makes this change purely additive. Easy to re-surface onRoomlater if that reads better.Registration is idempotent for equivalent schemas and throws for a name reused with a different shape. Compared structurally via
jsonSchema, not by object identity —Schemas.Map({...})mints a fresh object per call, so identity comparison would reject two modules declaring the same method with textually identical schemas, and would reject any code path that re-evaluates its definitions literal.Testing
test/sdk/network/requests.spec.ts, 29 cases driving a real clientRoomand serverRoomwired together through the platform's addressing rules (clients can only reach the server; the server broadcasts or targets). Covers the happy path, async handlers, falsy response bodies,RequestErrorforwarding, internal-error redaction, timeout, out-of-order concurrent requests, reply addressing, and schema conflict detection — plus, for the fixes above: a request issued while the room is disconnected (ticked well past its deadline, then connected, asserting it neither times out nor double-runs the handler), a server request with no target, a reply from a peer the request did not name, an oversized response, an unserializable request, two instances handling one method on one room, and a structurally-identical re-declaration.make test— 158 suites / 1211 tests, 0 failuresmake lint—@dcl/sdkcleanmake build— passes, including the playground-assets API report gatetsc --noEmiton@dcl/sdk— cleanNotes for review
checkPayloadcosts one extra serialization per message. The alternative was a back door onRoomthat accepts a pre-encoded buffer; an@internalmethod would be stripped from the emitted.d.tswhile still existing at runtime, which is a worse shape than paying for a second encode on a request/response path.network/server, deliberately: keeping this module outside the network transport's import cluster is worth more than sharing one number. Commented at the definition.