122 lines
5.8 KiB
Markdown
122 lines
5.8 KiB
Markdown
# WebSocket Client Robustness: Parse Guard, Reconnect Backoff, Dead-Channel Fallback
|
||
|
||
Priority: **High** · Effort: **Low–Medium** · Risk: **Low**
|
||
|
||
## Problem
|
||
|
||
The WebSocket client in `public/channelSync.js` is brittle in three ways that can leave the UI silently desynced or hammering a dead server/channel:
|
||
|
||
1. **`JSON.parse(e.data)` has no try/catch** (`channelSync.js:262`). A single malformed or partial frame throws, the `onmessage` handler aborts, and the client stops processing all further state updates while the socket appears "open."
|
||
2. **Reconnect has no backoff, no jitter, no max-retries** (`channelSync.js:353-369`). A down server produces a fixed 2–3s reconnect storm with no cap.
|
||
3. **No fallback to the default channel**. If the current channel is deleted server-side while the client is disconnected, every reconnect 404s and the client loops forever against a dead channel ID.
|
||
|
||
Separately, the message dispatcher falls through to `M.handleUpdate(data)` for any unknown `data.type` (`channelSync.js:346`), so a new server message type silently corrupts state. Worth hardening in the same pass.
|
||
|
||
## Affected Locations
|
||
|
||
- `public/channelSync.js:262` — unguarded `JSON.parse`
|
||
- `public/channelSync.js:346` — `handleUpdate` fallback for unknown types
|
||
- `public/channelSync.js:353-369` — reconnect logic
|
||
- `public/channelSync.js:245-261` — socket setup (reference for structure)
|
||
|
||
## Implementation Plan
|
||
|
||
### Step 1 — Wrap `JSON.parse` in try/catch
|
||
|
||
```js
|
||
M.ws.onmessage = (e) => {
|
||
let data;
|
||
try {
|
||
data = JSON.parse(e.data);
|
||
} catch (err) {
|
||
console.warn("[channelSync] Dropping malformed WS frame:", err);
|
||
return;
|
||
}
|
||
// ... existing dispatch ...
|
||
};
|
||
```
|
||
|
||
Keep behavior: a bad frame is dropped, the socket stays open, subsequent good frames still process.
|
||
|
||
### Step 2 — Strict dispatch with explicit unknown-type handling
|
||
|
||
Replace the implicit fall-through to `M.handleUpdate(data)`. Make the dispatch explicit:
|
||
|
||
```js
|
||
switch (data.type) {
|
||
case "channel_list": /* ... */ break;
|
||
case "switched": /* ... */ break;
|
||
case "track": /* fallthrough */
|
||
case "state": M.handleUpdate(data); break; // explicit state-shape types only
|
||
// ... all other known types ...
|
||
default:
|
||
console.warn("[channelSync] Unknown WS message type:", data.type, data);
|
||
}
|
||
```
|
||
|
||
(If the server currently sends `type` values other than the ones handled, gather the full set first via `grep` for `"type":` / `type: "` in the server broadcast paths and list them in this switch.)
|
||
|
||
### Step 3 — Reconnect with exponential backoff + jitter + max attempts
|
||
|
||
Track reconnect state on `M` (e.g. `M.wsReconnect = { attempts: 0, timer: null }`). On close:
|
||
|
||
```js
|
||
M.ws.onclose = () => {
|
||
M.ws = null;
|
||
// clear any user-facing "connected" state
|
||
if (!M.wantSync) return;
|
||
|
||
const recon = M.wsReconnect;
|
||
recon.attempts++;
|
||
const base = 1000; // 1s
|
||
const cap = 30000; // 30s max
|
||
const exp = Math.min(cap, base * 2 ** recon.attempts);
|
||
const jitter = Math.random() * 500; // 0–500ms
|
||
const delay = exp + jitter;
|
||
|
||
recon.timer = setTimeout(() => {
|
||
connectChannel(M.currentChannelId);
|
||
}, delay);
|
||
};
|
||
```
|
||
|
||
Reset `recon.attempts = 0` on a successful `open`. Cancel `recon.timer` on any explicit/manual `connectChannel` call to avoid double-connects.
|
||
|
||
**On max attempts**: rather than giving up entirely (a music app should keep trying), cap the delay at 30s but keep retrying. Optionally surface a "reconnecting…" indicator after the first few failures so the user knows the stream is stale.
|
||
|
||
### Step 4 — Dead-channel fallback
|
||
|
||
In `connectChannel`, if the upgrade/open fails or the server responds with a channel-not-found error message (the server already sends `{ type: "error", message: "Channel not found" }` per `websocket.ts:50`), fall back to the default channel:
|
||
|
||
```js
|
||
// inside onmessage error handler:
|
||
if (data.type === "error" && /not found/i.test(data.message)) {
|
||
console.warn(`[channelSync] Channel ${M.currentChannelId} not found, falling back to default`);
|
||
// fetch default channel id from /api/channels (isDefault === true)
|
||
const def = await fetchDefaultChannelId();
|
||
if (def && def !== M.currentChannelId) {
|
||
M.currentChannelId = def;
|
||
M.saveChannelId(def);
|
||
connectChannel(def);
|
||
M.showToast("This channel no longer exists — switched to the default channel.");
|
||
return;
|
||
}
|
||
}
|
||
```
|
||
|
||
Add a small helper `fetchDefaultChannelId()` that GETs `/api/channels` and returns the `id` where `isDefault === true`. Cache it on `M.defaultChannelId` after first successful list load so we don't re-fetch on every failure.
|
||
|
||
## Validation
|
||
|
||
- **Malformed frame**: from server console or a test, `ws.send("not json")` to a client — confirm the client logs a warning and continues processing subsequent valid frames (state still updates).
|
||
- **Server kill / restart**: stop the server, confirm reconnect attempts grow with backoff (log each attempt + delay) rather than firing every 2s. Restart, confirm the client reconnects and `attempts` resets.
|
||
- **Channel deletion while disconnected**: note a non-default channel ID, stop server, delete the channel row from `blastoise.db`, restart server, ensure the client reconnects → gets "not found" → falls back to the default channel and shows the toast.
|
||
- **Dispatch**: inject an unknown `{ type: "future_thing" }` message server-side (or via a debug WS send) and confirm it logs a warning and does **not** call `handleUpdate`.
|
||
|
||
## Risk / Rollback
|
||
|
||
- All changes are additive hardening; normal-path behavior is preserved.
|
||
- Backoff introduces up to a 30s worst-case reconnect delay — acceptable and better than a tight loop. If a deployment wants faster recovery, tune `base`/`cap`.
|
||
- Default-channel fallback adds one HTTP fetch on the error path only; cache the result to avoid repeats.
|
||
- Rollback = revert the file; no schema or wire-protocol change.
|