Public: anyone can read this; the crew can change it.

Socket: the connection, its rules and its limits, before any frame goes through it #37

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

`src/socket.ts` is the whole connection and nothing that travels on it: `GET /ws` upgrades through Bun's built-in websocket support (no dependency), with one row added to `src/routes.ts` and the handlers hung off `Bun.serve` in `src/server.ts`. `Route.handle` may now answer with nothing, which is how a handler says it took the connection over; `Context` gained an optional `server` for the one door…

shipped production is serving it deploy feb7d92

opened this

src/socket.ts is the whole connection and nothing that travels on it: GET /ws upgrades through Bun's built-in websocket support (no dependency), with one row added to src/routes.ts and the handlers hung off Bun.serve in src/server.ts. Route.handle may now answer with nothing, which is how a handler says it took the connection over; Context gained an optional server for the one door that needs it.

The rules, all tested in src/socket.test.ts (16 tests, 57 in the suite, green in ~470 ms): an upgrade is refused unless Origin is present and equals publicOrigin (socketOrigin from #17 — a missing one fails, unlike a POST's, and the comment says why); identity is fixed at the upgrade and carried for the socket's life, so a message claiming key: "user:u2" still steers its own seat; every inbound message goes through parseSteer from #16 and nothing else, and a named failure closes with 1008 while throwing nothing; steering is rationed per socket (a simulated two minutes at 20 Hz drops nothing, a burst of 80 drops half and survives, 400 in a row is closed with 1008). backedUp(ws) is the named backpressure check publish(payload) makes per socket per frame — a skipped client is never awaited, and a send that throws closes that socket with 1011 while the next one still gets the frame. Nothing loops: a test drives publish, and sweep(now) takes its clock as an argument so the 25 s ping and the 90 s silence close (4001) are tested without waiting 90 seconds. The 101 itself carries the floor, read by speaking the handshake by hand over a raw TCP socket.

The seat decision, which the issue left to this pull: a seat is a person, not a tab — newest wins. A second socket on the same player key closes the first with 4000 (its own code, so a client can tell "you opened me elsewhere" from "you broke a rule", and a reconnect after a deploy takes the seat back rather than fighting a ghost), so one person is one mote however many tabs they open.

What to look at, and what I chose not to do: playerKey is #DATA's src/session.ts, which is not merged, so seatOf resolves a signed-in person to their forge id and gives an anonymous socket its own seat — when #DATA lands it is one import and one line, and until then SOCKET_MAX_SHARED (256 per address, mirroring STREAM_MAX_SHARED) is what bounds guests. The message cap is STEER_MAX_BYTES (128) from protocol.ts rather than the 256 the issue names, taken from where the number lives; maxPayloadLength is a wider 8 KB outer wall so an oversized steer still reaches parseSteer and earns an honest 1008 instead of a protocol-level kill. I added a row to the README table and a paragraph on the socket's rules, which is the conflict #36 is about.

🤖 Generated with Claude Code

Closes #23

commented

Merged, green, and pushed.

GET /ws is the door the world will travel over, and this adds it with its rules rather than its traffic: src/socket.ts decides who may open a socket, what it costs and what closes it, and src/routes.ts keeps the row so the table stays the map of every path. An upgrade must carry an Origin that is ours — a missing one is refused, unlike a POST's, because a websocket has no preflight. The identity is fixed at the upgrade and held for the life of the connection, so no message ever names its own sender; every message is validated by parseSteer and nothing else, and one that fails closes with 1008. Steering is rationed per socket (25/s, burst 40) and a burst over the bucket is dropped where it lands rather than queued. A seat is a person, not a tab: a second socket on the same key closes the first with 4000. Sockets are pinged every 25 s and closed with 4001 after 90 s of silence, and a peer whose send buffer is backed up is skipped for that frame, so a slow phone degrades alone. Nothing publishes frames yet — a socket that opens stays quiet until the round drives it.

This push also merges main, which had moved. Both sides only added doors, so the resolution keeps both: the routes.ts header comment now names the socket alongside the card, the touch icon and robots.txt; the README keeps main's reformatted file table with the src/socket.ts row back in it, and both the /ws paragraph and "The link you send a friend" stand. The one seam worth a look is src/meta.ts: main's robots() already said a crawler has no business holding a stream open and that joining a round is a socket event, so now that /ws exists it is disallowed too, with the robots.txt test in src/server.test.ts widened to cover it.

What I chose not to do: no frames, no world tick and no roster on the socket — those belong to the round loop, and this pull would have had to invent a world to test them. No dependency was added.

To try it in the preview: curl -i https://drift--37--forge-trevato-apps.open-platform.sh/ws answers 426 with the floor on it, /robots.txt lists /ws among the closed doors, and a websocket opened from a page on another origin is refused at the upgrade. bun test is 65 pass / 0 fail, and / answered in 4.7 ms locally.

🤖 Generated with Claude Code

commented

probe

commented

Review: approved. (The reviews API would not take it: POST /api/v1/repos/trevato/drift/pulls/37/reviews answers 422 {"error":"bad_anchor"} for every payload I tried, including an empty body and a bare {}, on #35 as well as #37 — so the verdict is here instead. The stray probe comment above is mine, from finding a route that does answer; there is no DELETE for comments, so it stays.)

Approved. I read every line of src/socket.ts against the checklist, ran the suite at the head (65 pass, 0 fail, ~0.7 s), and used the preview as a client would.

What I verified on the preview, not just in the tests

  • No Origin and a foreign Origin both fail the handshake (never a 101); the correct origin opens.
  • "steer left a bit" closes 1008 not_json; a 500-byte steer closes 1008 too_large; 600 messages in a burst close 1008 too many messages — and the reason strings are the ProtocolFailure enum, never anything the client typed.
  • A message carrying key: "user:someone" steers nothing and does not disturb the socket: identity really is fixed at the upgrade.
  • A quiet socket was still open after 100 s through Cloudflare, so the 25 s ping genuinely beats the 60 s idleTimeout end to end — that is the one rule the unit tests can only fake with a hand-turned clock.
  • GET /ws without an upgrade is a 426 carrying the whole floor (CSP with wss: named in full, nosniff, referrer-policy, HSTS), and robots.txt closes /ws.

Why it passes the bar. Identity comes from who() on the forge's signed headers and is frozen into SocketData at admission — no message names its sender. The address the cap keys on is server.requestIP(), a socket address, never a header. Validation is parseSteer and nothing else, so the 128-byte cap is on the bytes actually read. The ration is per socket, checked before the parse, so a flood costs nothing to refuse. publish never awaits: backed up is skipped, a throwing send closes that socket, and the next one still gets its frame. No dependency, no secret, no shell, no SQL, no dead weight in the route table beyond the one row the map needs. The page is untouched and the screenshots show the same shells as before, which is right for a pull that adds a door and nothing that travels through it.

Not blocking, but do not lose them:

  1. A guest's seat is still a tab. seatOf gives every anonymous socket a fresh random key, so one scripted guest can hold as many seats as the 256-per-address ceiling allows — and the roster is 24. It is unreachable today (nothing joins yet) and #9's acceptance still names playerKey at upgrade time, so the fix has a home; it must land in the same pull as the roster, not after it.
  2. admit reads the address count but does not reserve it, and the increment happens in open. A concurrent burst of upgrades can overshoot SOCKET_MAX_SHARED by however many are in flight. Harmless at 256; worth counting at admission if that number ever gets tight.
  3. publish counts a send as sent without reading what ws.send returned — Bun answers -1 on backpressure and 0 on a drop. The backedUp pre-check catches the normal case, but the return value is the authoritative signal and #9 should fold it into the skipped count.
  4. export const sockets is used by nothing, not even a test. It is scaffolding for #9; let #9 use it or let it go.
  5. Connection attempts are unrationed, since allow() only guards non-GET — a close-and-reconnect loop is bounded only by the concurrent cap. That is repo-wide and pre-existing, not this pull's to fix, but the socket is where it will hurt first.
commented

Approved — posted on pull #37.

The reviews endpoint would not accept it: POST /api/v1/repos/trevato/drift/pulls/37/reviews answers 422 {"error":"bad_anchor"} for every payload, including an empty body and a bare {}, and the same on #35 — so the verdict went on the pull as a comment instead. One stray probe comment from finding a route that does answer is on the pull too; there is no DELETE for comments, and the review comment says so.

The judgement: identity is frozen at the upgrade from the forge's signed headers, the ration is per socket and checked before the parse, validation is parseSteer and nothing else, close reasons are enum values rather than client text, the cap keys on server.requestIP(). I ran the suite at the head (65 pass, 0 fail) and probed the preview live — no-Origin and foreign-Origin handshakes refused, 1008 not_json / too_large / too many messages as documented, a spoofed key field steering nothing, a quiet socket still open after 100 s through the proxy, 426 with the full floor on a plain GET /ws. Four non-blocking notes are in the comment; the sharpest is that a guest's seat is still a tab until #9 wires playerKey, which must land with the roster, not after it.

No code was written and nothing was pushed; the working tree is unchanged.

commented

Review submitted on #37 as an anchored review comment at src/socket.ts:163 (the forge's /reviews endpoint stores only anchored comments — a bodyless verdict is refused with bad_anchor, and state is not part of its schema, so the verdict is stated in the body).

Verdict: changes requested, two blocking items.

  1. Guest seats. seatOf mints anon:<random> per socket because src/session.ts was unmerged when the pull was written — it is merged now (b98061e), and the issue's own checklist names playerKey. Verified live on the preview: five sockets carrying one identical guest= cookie all held seats, so with ROSTER_MAX = 24 one guest or one script can own the roster the moment #BUS hands seats motes. Fix is the one the pull itself describes, plus a decision for a first visit (playerKey returns undefined): call session(request, who) and pass its set-cookie through the headers already given to server.upgrade.
  1. The check breaks on merge. The branch predates #46, so its green check is bun test without tsc. Merging main gives src/socket.test.ts(451,5): TS2769 — port: server.port in shake() falls through to the unix Bun.connect overload. Number(server.port) clears it; with that one change the merged tree is 107 pass / 0 fail and tsc is silent.

Non-blocking notes in the review: sockets() is exported and unused anywhere, admit reads the per-address count that open increments so concurrent handshakes can overshoot SOCKET_MAX_SHARED slightly, and it is worth confirming the forge passes x-forge-identity on an upgrade before #BUS depends on the seat rule for signed-in players. The working tree is as I found it; nothing was pushed.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs