146 lines
7.6 KiB
Markdown
146 lines
7.6 KiB
Markdown
---
|
|
id: review-001-ws-session-limits
|
|
name: WS session lifecycle — idle timeout, shutdown, teardown, caps (WS-01, WS-07..WS-10)
|
|
status: completed
|
|
depends_on: [review-001-ws-eof-signal]
|
|
scope: narrow
|
|
risk: medium
|
|
impact: component
|
|
level: implementation
|
|
tags: [websocket, review-001]
|
|
---
|
|
|
|
## Description
|
|
|
|
Review 001 WS subsystem lifecycle/staleness findings over
|
|
`src/websocket/upgrade.rs` and `byte_adapter.rs`:
|
|
|
|
- **WS-01 (major)**: the single demux loop means one dribbled chunk
|
|
stalls all channels indefinitely — alkcall's demux has no read timeout
|
|
and a peer header `[ch=0][len=16 MiB]` + one byte/ minute parks the
|
|
allocation and hangs every outstanding channel-0 call (Sub/Pub pendings
|
|
until the socket dies). Add a configurable idle timeout on the WS read
|
|
that closes with 1001 on staleness (deployment knob; default bounds the
|
|
stall).
|
|
- **WS-07**: `poll_shutdown` drops a *fresh clone* of `write_tx`
|
|
(`byte_adapter.rs:269-276`) so the channel never closes and the
|
|
documented `ws_sink.close()` never runs at shutdown — the `from_wss`
|
|
path never emits a WS Close frame at all. Drop the held sender.
|
|
- **WS-08**: `_pumps` is dropped immediately (`upgrade.rs:36`); the
|
|
documented forced-teardown lever `WsPumps::abort()` is never callable
|
|
on the server path. Retain the handle so a stuck session is evictable
|
|
in-crate.
|
|
- **WS-09**: no cap on WS sessions (post-auth DoS); the assembly layer
|
|
cannot add one because the route is built inside `HttpAdapter`. Add a
|
|
semaphore in `ws_upgrade_handler` (configurable; document the default).
|
|
- **WS-10**: dispatcher/mux tasks outlive a failed session task
|
|
(`upgrade.rs:65-97`) — self-healing but a peer-behavior-tied leak
|
|
window. Document the semantics (the cheap fix) or tie task lifetimes
|
|
to the session.
|
|
- Fold-in from review-001-hyper-server-knobs: SRV-10's policy-injection
|
|
point (channel cap) belongs here if that task doesn't land it.
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [x] Idle timeout bounds a dribble stall (test: dribbling peer's channels fail/bound rather than hang forever)
|
|
- [x] `shutdown()` causes a WS Close frame to the peer (test)
|
|
- [x] `_pumps` retained; `abort()` reachable on the server path
|
|
- [x] Session concurrency cap configurable in `HttpAdapter`; enforced (test)
|
|
- [x] Detached-task lifetime semantics documented (WS-10)
|
|
- [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass
|
|
|
|
## References
|
|
|
|
- docs/reviews/001-initial-implementation-review.md (Part B, WS-01, WS-07..WS-10)
|
|
- docs/architecture/decisions/048-websocket-native-session-not-gateway.md
|
|
|
|
## Notes
|
|
|
|
> Agent fills during implementation. Takes the EOF-signal task first
|
|
> (same files, and the lifecycle knobs build on the lossless signal).
|
|
|
|
## Summary
|
|
|
|
All five lifecycle findings fixed in `src/websocket/` (+ `server/`
|
|
builder plumbing, one test-side touchpoint):
|
|
|
|
- **WS-07** (`websocket/byte_adapter.rs`): `poll_shutdown` dropped a
|
|
*fresh clone* of `write_tx` — the pump-side channel never closed, so
|
|
the write pump's trailing `ws_sink.close()` (the WS Close frame at
|
|
shutdown) never ran. Mechanics of the fix: `WsByteStream.write_tx`
|
|
became `Option<Sender<WriteMsg>>`; `poll_shutdown` **takes** the held
|
|
sender (no clone). Both senders must be gone for the queue to close —
|
|
the read pump holds the other clone until its loop ends (peer close /
|
|
read error / idle timeout), which is the documented close sequencing.
|
|
The write pump then drains the queued bytes and closes the sink.
|
|
(Test: `tungstenite_shutdown_closes_the_write_sink_toward_the_peer`.)
|
|
- **WS-08** (`websocket/upgrade.rs` + `server/state.rs`,
|
|
`server/adapter.rs`): retention point is a **shared `WsSessions`
|
|
registry** — `Arc` of a `parking_lot` map (`u64 → Arc<WsPumps>`)
|
|
carried in `RouterState` and lifted into the handler via the new
|
|
`SessionState` `FromRef` substate; a `WsSessions` request extension
|
|
overrides the state instance (mirrors the SRV-10 `ChannelsPolicy`
|
|
seam; extending alkcall's `ChannelLifecyclePolicy` itself was not
|
|
possible — the trait is owned by the call crate). The session task
|
|
registers its pumps through a self-removing guard, so an entry
|
|
exists exactly for the session's lifetime; `WsSessions::abort()`
|
|
force-ends the pump tasks of every live session (eviction lever) and
|
|
`HttpAdapter::ws_sessions()` hands the shared instance to the
|
|
assembly layer. (Test:
|
|
`ws_sessions_registry_tracks_and_aborts_live_sessions`.)
|
|
- **WS-09** (`websocket/upgrade.rs`, `server/adapter.rs`): a
|
|
`tokio::sync::Semaphore` cap on concurrent WS sessions, acquired
|
|
post-auth / pre-upgrade in `ws_upgrade_handler` with
|
|
`try_acquire_owned`; exhausted → **503 Service Unavailable**. The
|
|
permit moves into the upgrade closure and is held for the session's
|
|
lifetime (ended sessions free their slot). Configurable via
|
|
`HttpAdapter::with_ws_max_sessions(usize)`; default
|
|
[`DEFAULT_WS_MAX_SESSIONS`] = **64** (documented const). One
|
|
semaphore per adapter, built once (an earlier per-request
|
|
construction bug would have made the cap a no-op — caught by the
|
|
acceptance test). (Tests: `session_cap_rejects_over_limit_with_503_and_frees_slots_on_end`.)
|
|
- **WS-01** (`websocket/byte_adapter.rs`: `run_read_pump`; knob in
|
|
`upgrade.rs`/`server/adapter.rs`): the read pump's next-message await
|
|
is wrapped in `tokio::time::timeout` **inside** the loop (the
|
|
consolidation shape untouched — `on_end` still fires exactly once
|
|
after the loop, so the from_wss watch+monitor+sweep machinery is
|
|
unchanged). Staleness sends the **1001 GoingAway** close
|
|
([`WS_GOING_AWAY`]) to the peer and ends the read loop → EOF → the
|
|
demux clears channels and fails pendings as a normal connection end.
|
|
The window resets per inbound WS message (a dribble of messages
|
|
within the window never trips; only a true stall does). Knob:
|
|
`HttpAdapter::with_ws_idle_timeout(Option<Duration>)` (`None`
|
|
disables); default [`DEFAULT_WS_IDLE_TIMEOUT`] = **60 s**. The split
|
|
functions gained `_idle` variants
|
|
(`split_ws_to_bytes_idle`/`split_tungstenite_to_bytes_idle`); the
|
|
old names keep the default. (Tests:
|
|
`idle_read_timeout_closes_a_stalled_connection_with_goingaway` —
|
|
stalled peer gets the 1001 + EOF signal within the knob;
|
|
`idle_read_timeout_resets_on_traffic` — a dribble of one message per
|
|
half-window keeps the connection alive across many windows.)
|
|
- **WS-10** (`websocket/upgrade.rs`, docs-only): new module-doc section
|
|
"Detached task lifetime semantics" — the pump and channel-0
|
|
dispatcher tasks are detached by design and outlive a failed session
|
|
task; they self-heal (each ends when its stream half closes), the
|
|
leak window is bounded by peer behavior and (when configured) the
|
|
WS-01 idle timeout, and every session's pumps stay force-evictable
|
|
via the WS-08 registry.
|
|
|
|
Also: `src/adapters/to_mcp.rs` test
|
|
`search_honors_query_substring_filter` asserted a hard-coded operation
|
|
order over a HashMap-derived upstream list (pre-existing flake on
|
|
main, fails ~1/3 in isolation, unrelated to WS); made the assert
|
|
order-independent (sorted).
|
|
|
|
Knob defaults: session cap 64 (`DEFAULT_WS_MAX_SESSIONS`), idle-read
|
|
timeout 60 s (`DEFAULT_WS_IDLE_TIMEOUT`). Pumps-handle retention: the
|
|
`WsSessions` registry in `RouterState` (`HttpAdapter::ws_sessions()`),
|
|
override via the `WsSessions` request extension. WS-07 mechanics: the
|
|
held `write_tx` is taken (Option) at shutdown — no clone — so the
|
|
pump-side close (drain + `ws_sink.close()` → WS Close frame) runs once
|
|
the read pump's sentinel clone also drops.
|
|
|
|
Verification: `cargo test` 300 passed; `cargo test --all-features` all
|
|
green (371 lib + ws/from_mcp/full-surface suites); `cargo clippy
|
|
--all-targets -- -D warnings` and `cargo clippy --all-features
|
|
--all-targets -- -D warnings` clean; `cargo fmt --check` clean. |