From 6b0e4db9123ffc1854b80f3b135e63c255c8450b Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Sat, 29 Aug 2026 13:51:23 +0000 Subject: [PATCH] docs(tasks): review-001 ws-session-limits remediation complete --- .../websocket/review-001-ws-session-limits.md | 98 +++++++++++++++++-- 1 file changed, 90 insertions(+), 8 deletions(-) diff --git a/tasks/websocket/review-001-ws-session-limits.md b/tasks/websocket/review-001-ws-session-limits.md index 89262ff..4766c55 100644 --- a/tasks/websocket/review-001-ws-session-limits.md +++ b/tasks/websocket/review-001-ws-session-limits.md @@ -1,7 +1,7 @@ --- id: review-001-ws-session-limits name: WS session lifecycle — idle timeout, shutdown, teardown, caps (WS-01, WS-07..WS-10) -status: pending +status: completed depends_on: [review-001-ws-eof-signal] scope: narrow risk: medium @@ -42,12 +42,12 @@ Review 001 WS subsystem lifecycle/staleness findings over ## Acceptance Criteria -- [ ] Idle timeout bounds a dribble stall (test: dribbling peer's channels fail/bound rather than hang forever) -- [ ] `shutdown()` causes a WS Close frame to the peer (test) -- [ ] `_pumps` retained; `abort()` reachable on the server path -- [ ] Session concurrency cap configurable in `HttpAdapter`; enforced (test) -- [ ] Detached-task lifetime semantics documented (WS-10) -- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass +- [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 @@ -61,4 +61,86 @@ Review 001 WS subsystem lifecycle/staleness findings over ## Summary -> Filled on completion. \ No newline at end of file +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>`; `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`) + 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)` (`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. \ No newline at end of file