relay: SetReadLimit on WSConn — bound per-frame size on both forwarders

[ilmoniemi]

User Story

As a relay operator, I want every WebSocket frame the relay reads from a peer bounded by a deliberate per-frame size cap, so that a misbehaving or malicious phone or binary cannot pin memory in the read buffer or amplify backpressure across forwarders by sending oversized frames.

Context

docs/knowledge/features/ws-conn-adapter.md § Out of scope explicitly flags this as a follow-up:

No per-message size cap on Read. Inherited from *websocket.Conn's default (nhooyr's 32 MiB read limit). A deliberate SetReadLimit policy is a follow-up so it covers both forwarders.

32 MiB per frame is too generous for a phone↔binary message channel where envelope payloads are typically well under 64 KiB. Phone messages are typed text + structured envelopes; backfill chunks per the protocol spec are bounded; image/blob messages (if any) flow out-of-band.

Setting the cap at WSConn construction makes the policy cover every reader through the adapter — both the phone-side forwarder (StartPhoneForwarder, #25) and the binary-side forwarder (StartBinaryForwarder, #26, now landed) — without each call site needing to know.

Acceptance Criteria

• A per-frame read-size cap is configured on every *websocket.Conn wrapped by WSConn, before any Read is performed against it. The mechanism is *websocket.Conn.SetReadLimit (or an equivalent applied at WSConn construction time).
• The cap is a single policy value defined as a literal in cmd/pyrycode-relay/main.go and threaded through the existing ServerHandler and ClientHandler constructors (same pattern as the 30*time.Second grace duration on ServerHandler). One literal in one place; no package-level constant in internal/relay.
• WSConn.Read returns a non-nil error when the peer sends a frame whose payload exceeds the cap, AND the underlying connection is closed by the library (no further reads succeed). The specific error type is library-dependent and is NOT asserted on — only the non-nil contract.
• The cap value is justified in the ticket spec by reference to pyrycode/pyrycode/docs/protocol-mobile.md § Message envelope; 256 KiB is the proposed starting value but the architect may revise after deriving the upper bound from the protocol spec.
• A unit test in internal/relay/ws_conn_test.go (using the existing startEcho/httptest.NewServer harness) verifies that a frame exceeding the cap surfaces as a non-nil Read error on the receiving WSConn and that subsequent reads fail. A second test verifies that a frame at-or-below the cap is delivered intact.
• On /v1/client, when the phone sends an oversized frame, StartPhoneForwarder returns; the handler's existing defer { UnregisterPhone; Close; log } runs unchanged — i.e. no new cleanup paths are introduced. Verifiable by inspection of client_endpoint.go (no defer additions) and by the existing forwarder test pattern continuing to pass.
• make vet, make test -race, and make build are clean.

Technical Notes

• Construction-site application is the natural seam: NewWSConn is the choke point through which both /v1/server and /v1/client reach the wire. Applying at handler entry (i.e. calling c.SetReadLimit between websocket.Accept and NewWSConn) would also work but duplicates the call across handlers; thread the value into NewWSConn instead.
• Test call sites that pass *WSConn (ws_conn_test.go, heartbeat_test.go, any forward-test fakes) will need their NewWSConn invocations updated for the new signature. These are mechanical one-line edits, not substantive changes.
• nhooyr.io/websocket's SetReadLimit documented behaviour: when exceeded, the next Read returns a non-nil error and the library closes the connection with StatusMessageTooBig (1009). The relay does not need to emit its own close code — the library handles it.
• Out of scope (separate tickets):
  • Per-IP / per-server-id rate limiting
  • Slow-loris hardening on the WS upgrade handshake
  • Differentiating phone vs. binary caps (single value covers both for now; revisit only if binary-side framing turns out to need a different bound in practice)
  • Making the cap operator-configurable via flag (current pattern is literal-at-wiring-site; promote only when a second operator needs a different value)

Size Estimate

S — single-file behavioural change in ws_conn.go (SetReadLimit call + signature change), single-literal wiring in main.go, mechanical one-line signature updates at four existing call sites (server_endpoint.go, client_endpoint.go, two test files), and one new test pair in ws_conn_test.go. Estimated <100 lines of production code; tests scale linearly.

The work passes the one-sentence-no-"and" test: "Apply a per-frame read-size cap at WSConn construction, threaded from the wiring site."

[ilmoniemi]

🤖 Product Owner — creates structured issues

po agent has completed work on this ticket.

Agent output (click to expand)

Refined #29 to standard format and applied `size:s`.

**Summary of refinement:**
- Converted Why/What/Suggested-cap structure into User Story / Context / AC / Technical Notes / Size Estimate
- Tightened acceptance criteria into 7 testable items (cap mechanism, single-literal wiring, Read error contract, justification source, two unit tests, no-new-cleanup contract on `/v1/client`, build/test cleanliness)
- Sized **S**: behavioural change is concentrated in `ws_conn.go` + `main.go`; four call-site updates are mechanical signature edits. Passes the one-sentence-no-"and" test.
- Preserved `security-sensitive` label (header validation / internet-exposed read path qualifies). Did not add `ready:po` — dispatcher will do that automatically.
- Kept the original "Out of scope" list and the `protocol-mobile.md` derivation note so the architect picks them up.

Note: the prompt declared "rework" mode, but the ticket has zero comments and no `needs-rework:*` label — there was no prior rework signal to address, so I treated it as a fresh refinement.

Ready for human review. Move to the next column when approved.

[ilmoniemi]

Blocked by #26: overlapping changes to internal/relay/server_endpoint.go.

Architect file-overlap check (per agent-definition policy under WIP=N dispatch): both this ticket and #26 (currently in flight via PR #33, label wip:documentation) modify internal/relay/server_endpoint.go.

• relay: binary-side frame forwarder — unwrap routing envelope and dispatch to addressed phone #26 replaces the CloseRead+<-Done() body block inside ServerHandler with StartBinaryForwarder(...) (lines ~100–113).
• relay: SetReadLimit on WSConn — bound per-frame size on both forwarders #29 will modify the ServerHandler signature (add readLimit int64 param at line 35) and the NewWSConn call site at line 59.

The textual diffs land on different lines of the same function, so git's three-way merge would likely succeed without conflict — but the file-overlap rule is intentionally mechanical (any shared file → block) to avoid case-by-case judgement under concurrent dispatch. The cost of deferring is small; #33 is close to merge.

Will re-run once #26 lands: feature branch is created fresh from main at that point, so the spec is written against the post-#26 state of server_endpoint.go (binary-side forwarder already wired) and threads the readLimit through both forwarder construction sites simultaneously — exactly the "covers both forwarders" framing the issue body already calls out.

No other content changes needed on the ticket body. Adding needs-rework:po so the ticket routes back to Backlog; auto-advances to In Architecture once #26 closes.

[ilmoniemi]

🤖 System Architect — defines Go interfaces, data flows, concurrency patterns

architect agent flagged issues on this ticket → rework by po.

Agent output (click to expand)

Blocked #29 on #26 and routed back to PO. No spec written, worktree untouched.

**Summary:** Architect file-overlap check tripped — both #29 and #26 (in flight via PR #33, `wip:documentation`) modify `internal/relay/server_endpoint.go`. Even though the diffs land on different lines (#29 changes `ServerHandler` signature at line 35 + `NewWSConn` call at line 59; #26 replaces the body block at lines ~100–113), the rule is mechanical for safety. Set `blockedBy(#29, #26)` via GraphQL, posted a comment explaining the deferral, swapped `wip:architect` → `needs-rework:po`. Ticket will auto-advance back to In Architecture when #26 closes, and the spec then gets written against the post-#26 state so `readLimit` threads through both forwarder wiring sites cleanly.

Needs rework by po. See agent findings above.