1 Commits

Include renames

Author	SHA1	Message	Date
Alexander PetricandClaude Opus 5.5	8caf414301	fix(multiplayer): a malformed frame from one client no longer exits the server (#11353) ... * fix(multiplayer): don't drop client messages during cold-start token verification `wss.on('connection')` awaits `verifyToken()` before `setupWSConnection()` attaches the 'message' listener. On a cold process that await includes the first `/api/debug/jwks` fetch (~30ms on ECS). A y-websocket client sends sync step 1 the instant the socket opens, and `ws` drops messages emitted with no listener attached, so that step 1 was lost and never answered with step 2 — the client's provider never became `synced`. Buffer messages from the moment the connection is accepted and replay them, in order, once `setupWSConnection()` has installed its handlers. Rejected connections drop the buffer and close with the same 4401/4403 codes as before. Also prefetch the public key at startup when WINDMILL_BASE_URL is set. That is insurance, not the fix: a connection arriving before the prefetch resolves still relies on the buffer. Adds `npm test` in multiplayer/ (node:test, no docker or backend needed) with a fake JWKS endpoint that answers with a delay, which holds the cold window open and makes the race deterministic; wired into the existing test_extra CI job. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(multiplayer): cap what an unauthenticated peer can buffer pre-auth Review follow-up. The pre-auth buffer was unbounded: `ws` sets no `maxPayload` here and the JWKS fetch has no timeout, so a peer that never authenticates could stream frames into memory for as long as `verifyToken` was stalled. Cap it at 32 frames / 1 MiB — a real client only has sync step 1 and its first awareness update in flight there — and close 1009 past that, dropping what was buffered. A socket closed during verification (by the peer, or by that cap) is no longer handed to setupWSConnection: it would be added to `doc.conns` with a 'close' listener that can never fire. The startup prefetch's .catch was dead code — getPublicKey() logs its own failures and resolves to null rather than rejecting. Test helper: pin REQUIRE_SIGNED_MULTIPLAYER_REQUESTS and BASE_INTERNAL_URL so an ambient value cannot turn the rejection tests into false passes; bind the JWKS server on port 0 instead of a released probe port, and retry the spawned server on EADDRINUSE; destroy still-delayed JWKS responses on teardown, since server.close() waits for in-flight requests. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test(multiplayer): gate the JWKS response instead of delaying it Review follow-up. The cold window was held open by a 1500 ms delay on the fake JWKS response, but that timer started when the startup prefetch reached the fake server, not when the client sent its first frame. A slow enough machine could load the key before the client connected, and the race test would then pass without ever exercising the buffer — a false pass. The fake JWKS server now parks every response until the test calls release(), so the server provably holds no key while the client is sending. The race test releases only after both frames are written to the socket, and asserts the server has not logged the key as loaded at that point; the flood test never releases until after the cap has closed the connection. What is left to wall-clock time is 250 ms for bytes already written to the socket to cross loopback into an otherwise idle server, rather than a window that had to cover process startup, connect and handshake. Also drops the prefetch precondition from the forged-token and flood tests so each test still maps to one behaviour. Suite runs in ~1.1s instead of ~5.3s. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(multiplayer): survive a malformed frame instead of exiting the process `setupWSConnection`'s message handler decoded whatever an authenticated peer put on the wire with no guard: `decoding.readVarUint`, `syncProtocol.readSyncMessage` and `awarenessProtocol.applyAwarenessUpdate` all throw on input they cannot parse, `ws` re-emits a listener's exception on the process, and server.mjs installs no `uncaughtException` handler. One bad frame from one client therefore killed the whole multiplayer server, taking every other document and every other client with it. Catch decode/apply failures, log the document, the client address and the error message (never the payload), and close only the offending connection with 1007 "invalid frame payload data". Frames that arrive once a connection is no longer OPEN are ignored, so the replay of the pre-auth buffer stops at the first refusal instead of applying the rest. docker/entrypoint-extra.sh made that outage permanent: on a service exit it logged a bare PID and then `wait`ed on the rest, so the container stayed up with a dead service and the health checks in front of it — which probe the LSP — saw nothing wrong. It now names the service that died, stops the others through the same shutdown path SIGTERM uses, and exits non-zero so the orchestrator replaces the container. The "no services enabled" branch still sleeps. Tests: multiplayer/test/malformed_frame.test.mjs covers four malformed payloads from an authenticated client and one replayed out of the pre-auth buffer, asserting the 1007 close, a live server process, an undisturbed bystander and a real edit still propagating. docker/test_entrypoint_extra.sh runs the real entrypoint in a container with stub services. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test(multiplayer): prove the replayed malformed frame really is buffered pre-auth Assert the server has not yet logged the loaded key when the frame is written, and give it the same in-flight margin as the cold-start tests before releasing the JWKS response, so the frame provably goes through the replay path rather than landing on an already-authenticated connection. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * ci(extra): run the entrypoint supervision tests in publish_extra The multiplayer unit tests already run there; the entrypoint test needs only docker and the checkout, so run it in the same job, before the image build. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(multiplayer): handle WebSocket protocol errors and bound the shutdown Two crash paths of the same class as the malformed-frame one, from review. `ws` fails a frame it cannot parse at the protocol level — an unmasked frame from a client, a reserved opcode, a bad RSV bit — inside its Receiver, before the application 'message' handler ever sees it, and `receiverOnError` ends with `websocket.emit('error', err)`. With no 'error' listener that is an unhandled EventEmitter error, so it exited the process just as a malformed payload did. (A raw socket error such as ECONNRESET does not: ws 8.21.3's `socketOnError` swallows those.) Add the listener on the accepted socket, before authentication so the pre-auth window is covered too, and one on the server. Log messages now go through `describeError`, which collapses whitespace and truncates, so nothing that reaches an error message can forge or flood a log line. `stop_services` ended in a bare `wait`. On the `docker stop` path dockerd provides the deadline; the "a service died" path signals itself, so a service that is wedged or slow to honour SIGTERM would hold the container open indefinitely — the state that path exists to prevent. Bound it: SIGTERM, wait SHUTDOWN_GRACE_SECS (10 by default), then SIGKILL the stragglers by name. Tests: multiplayer/test/socket_error.test.mjs (authenticated and pre-auth illegal frames, asserting a live process and continued service), a SIGTERM-ignoring stub scenario in docker/test_entrypoint_extra.sh, and that harness is now bounded throughout — `timeout -k` on foreground runs, a watchdog around the backgrounded ones, and an optional outer timeout on `docker run`. `--entrypoint bash` so the documented windmill-extra:test override runs the harness instead of the image's real entrypoint. The stubs publish a readiness marker and the dying one waits for them, removing a startup race in the harness. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(multiplayer): never log peer bytes, and keep a fatal server error fatal Two review findings on the previous commit, both mine to answer for. `describeError` collapsed whitespace, which is not enough. An error message is not always a fixed string: `applyAwarenessUpdate` runs `JSON.parse` on the peer's bytes and V8 quotes ~30 bytes of the offending input back verbatim, ESC included, so a peer could put terminal escapes and forged content into a log line. Strip everything outside printable ASCII instead, and say so where the comment previously claimed the messages were fixed strings. `wss.on('error')` was worse than the crash it replaced for one case: `ws` forwards the HTTP server's errors there, so a failed listen (EADDRINUSE) was logged and the process then exited 0 — a clean shutdown as far as anything upstream could tell. It now sets a non-zero exit code. Setting `process.exitCode` rather than calling `process.exit()` keeps the log line from being truncated. `openClient` in the test helpers now records the socket error it was already swallowing, so a failed connection reports its cause instead of surfacing as a bare `waitFor` timeout. Tests: a malformed awareness frame whose state is `x\x1b[2J OWNED THE LOG` added to the payload table, with every case now asserting exactly one refusal line and no control characters in it (1 fail before, 0 after, 3 runs); and a server that cannot listen must exit non-zero (1 fail before, 0 after, 3 runs). 14/14 on 5 consecutive runs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test(multiplayer): make the exit-status helper robust to spawn and stdio races Review follow-ups on runMultiplayerServerUntilExit. Wait for 'close', not 'exit': 'exit' fires when the child terminates, which can be before its stdio pipes are drained, and the caller reads the output. On the EADDRINUSE path the child writes one line and exits immediately after, which is exactly the shape that loses it. Listen for 'error' too. A child that fails to spawn emits neither 'exit' nor 'close', so the promise would never settle and the SIGKILL guard could not help. Report whether the guard fired, rather than leaving the caller to infer it from the exit signal: `signal` is null for every child exit on Windows, so a server that hung after the listen error would have looked like one that exited on its own. The test asserts on that flag instead. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test(multiplayer): assert the refusal line itself, and name hasSyncType for what it takes Two review nits on the test helpers. The `doc="..."` assertion searched the whole server log, where CONNECT and DISCONNECT also name the document, so it would have passed even if the refusal stopped naming anything. Every assertion about the refusal is now made against the refusal line, which the test already isolates, and it also checks the peer is named. `hasKind` took a sync sub-type but was named as if it took any message kind, and the two families overlap numerically (`syncStep1 === messageSync === 0`), so a caller passing the wrong one got a silently wrong answer. No runtime check can tell aliased numbers apart, so the fix is the name: `hasSyncType`, with the overlap spelled out where the constants are declared. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test(multiplayer): make the pre-auth tests prove which path they took The replay test could not establish that its frame went through the pre-auth buffer: a frame delivered after setup, into the live handler, produces the same close code and the same refusal line, so if the in-flight margin were ever missed the test would quietly become a duplicate of the main-loop cases rather than fail. server.mjs now logs REPLAY when, and only when, it replays a buffered pre-auth message — worth having on its own, since that path only runs when a client beat the JWKS fetch on a slow-starting instance — and the test asserts on it. Removing that log line turns the test red, which is the point. The socket-error pre-auth test gated on `jwks.requests >= 1`, which the startup warm-up already satisfies, so it proved nothing about the offender. What makes it the pre-auth case is that the JWKS response stays parked for the whole test; it now asserts the server never logged CONNECT, which is exact. `killedByTimeout` was set before the kill, so a child that exited on its own just before the timeout — with 'close' still pending on the stdio drain, the very window this helper waits for — would have been reported as killed. It now claims the rescue only when there was a live process to signal. 14/14 on eight consecutive runs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test(multiplayer): document the frame-recording contract in openClient ws hands every frame over as a Buffer under the default binaryType, text frames included, so recording them as Uint8Array is lossless for both. Worth stating: ws 7 delivered text frames as strings, where new Uint8Array(string) would have been a silent zero-fill, and the difference is not visible at the call site. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test(multiplayer): pin the close code for an illegal frame, and trim comments to the 4-line rule Both socket-error tests waited for a close and never checked what it was, so an abrupt 1006 teardown would have passed while the comment beside the payload claimed 1002. `ws` sends 1002 for an unmasked frame in both the authenticated and pre-auth cases, confirmed over repeated runs; that is now a named constant asserted in each test, mirroring malformed_frame.test.mjs. Changing the expected value turns both red. The comments added by this branch also ran past the four lines AGENTS.md allows, and several justified the change to a reader rather than stating the invariant. Condensed to the invariant, at the site that would break it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>	2026-09-25 17:12:47 +02:00