94 lines
4.5 KiB
Markdown
94 lines
4.5 KiB
Markdown
---
|
||
id: review-001-ws-pump-consolidation
|
||
name: Dedupe pumps + byte-based caps + write-side validation (WS-11, WS-04, WS-05, WS-06, HY-09)
|
||
status: completed
|
||
depends_on: []
|
||
scope: moderate
|
||
risk: medium
|
||
impact: component
|
||
level: implementation
|
||
tags: [websocket, review-001]
|
||
---
|
||
|
||
## Description
|
||
|
||
Review 001 findings where the correct fix is restructuring the duplicated
|
||
pump code in `src/websocket/byte_adapter.rs`, so they are done once
|
||
instead of twice (axum path `:119-180`, tungstenite twin `:299-364`):
|
||
|
||
- **WS-11**: ~60 lines of pump logic (read task, write task, close
|
||
handling) are copy-pasted between the axum and tungstenite paths while
|
||
the module doc claims "one implementation, both directions". Factor
|
||
over a generic sink/stream of messages. This also closes COV-03 (the
|
||
tungstenite twins are untested).
|
||
- **WS-04 + HY-09**: the write-side chunk parser does no length
|
||
validation (`:161-163`, twin `:343-345`) — non-chunk-framed bytes make
|
||
the parser silently wait to accumulate `8 + len` (up to ~4 GiB) from
|
||
misaligned offsets. Validate `len > MAX_CHUNK_LEN` → fail the stream
|
||
loudly; saturating add for the 32-bit overflow.
|
||
- **WS-05**: the write-side `pending` buffer is bounded in slots (64) but
|
||
not in bytes (~1 GiB worst case per connection with a slow sink).
|
||
Cap `pending` growth in bytes.
|
||
- **WS-06**: inbound per-connection memory bound is 64 slots × 64 MiB
|
||
(axum/tungstenite defaults) ≈ 4 GiB — `max_message_size`/
|
||
`max_frame_size` are never configured. Set explicit caps consistent
|
||
with the plan's ~1 MiB write-side intent.
|
||
|
||
## Acceptance Criteria
|
||
|
||
- [x] Pump bodies factored into one generic implementation (both paths share it)
|
||
- [x] Write-side rejects `len > MAX_CHUNK_LEN` with a stream error (test); no `8 + len` overflow path
|
||
- [x] Write `pending` byte-capped; inbound message/frame size explicitly configured (tests)
|
||
- [x] Tungstenite path exercised by the shared test suite (COV-03 backfilled)
|
||
- [x] 16 MiB round-trip and disconnect-cleanup tests still pass
|
||
- [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass
|
||
|
||
## References
|
||
|
||
- docs/reviews/001-initial-implementation-review.md (Part B, WS-04, WS-05, WS-06, WS-11; HY-09; COV-03)
|
||
|
||
## Notes
|
||
|
||
> Agent fills during implementation. Sequenced with (or after)
|
||
> review-001-ws-eof-signal so the dedup doesn't churn under it — both
|
||
> touch byte_adapter.rs.
|
||
|
||
## Summary
|
||
|
||
Restructured `src/websocket/byte_adapter.rs` in three commits:
|
||
|
||
- **WS-04/HY-09 (5024d99):** the write-side chunk parser validates the
|
||
8-byte header's length field against alkcall's `MAX_CHUNK_LEN`
|
||
(re-exported as `alkhttp::websocket::MAX_CHUNK_LEN`) with saturating
|
||
`8 + len`. Oversized header → the pump closes the socket (1011) and
|
||
signals the `AsyncWrite` half through a one-shot error channel, so
|
||
`poll_write` fails loudly (`InvalidData`, "chunk length exceeds
|
||
MAX_CHUNK_LEN") instead of silently waiting toward ~4 GiB. Tested on
|
||
the tungstenite path over a raw duplex pair.
|
||
- **WS-05/WS-06 (92cc11a):** `pending` is byte-capped at
|
||
`PENDING_BUFFER_CAP` (= `MAX_CHUNK_LEN + 8`, checked pre-extend and
|
||
post-parse; a smaller "e.g. 1 MiB" cap false-positives because the
|
||
alkcall mux passes each payload up to 16 MiB as one `AsyncWrite`
|
||
call — the cap is the exact bound well-framed traffic can occupy).
|
||
Both pumps fail loudly on breach. Inbound caps
|
||
`INBOUND_WS_MESSAGE_CAP`/`INBOUND_WS_FRAME_CAP` (both 1 MiB) set on
|
||
the axum upgrade (`max_message_size`/`max_frame_size`) and the
|
||
tungstenite dial (`connect_async_with_config`+`WebSocketConfig`);
|
||
frame cap = message cap because the path does not use WS
|
||
fragmentation and tungstenite rejects a single oversize frame before
|
||
reassembly. A read-task inbound error surfaces a 1011 close to the
|
||
peer + stream error (tested).
|
||
- **WS-11/COV-03 (1db0ea8 + e9ddf94):** one generic implementation
|
||
([`WsFraming`] trait + `run_read_pump`/`run_write_pump`) with two
|
||
flavor impls (axum/tungstenite); the EOF watch mechanism, `read_eof`
|
||
receiver, and from_wss monitor unchanged. Six byte-adapter unit
|
||
tests now exercise the tungstenite path directly (WS-04, WS-05,
|
||
well-framed passthrough, binary read, EOF retention, text→1002);
|
||
the shared ws_upgrade_session/ws_overlay_ops/from_wss suites cover
|
||
both paths end-to-end.
|
||
|
||
Verification per commit: `cargo test`, `cargo clippy --all-targets --
|
||
-D warnings`; final also `cargo test --all-features`, `cargo clippy
|
||
--all-features --all-targets -- -D warnings`, `cargo fmt --check` —
|
||
all green, including the 16 MiB round-trip and disconnect-cleanup
|
||
tests. |