Skip to content

Commit b484dc9

Browse files
committed
web: address gemini feedback
Signed-off-by: Peter Gadfort <gadfort@zeroasic.com>
1 parent aa966e3 commit b484dc9

2 files changed

Lines changed: 81 additions & 6 deletions

File tree

src/web/src/websocket-manager.js

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ export class WebSocketManager {
6565
this._cache = null;
6666
this._isConnected = false;
6767
this._shutdown = false; // set by server "shutdown" message
68+
this._livenessTimer = undefined; // set by _startLivenessMonitor
6869
this._startLivenessMonitor();
6970
this.connect();
7071
}
@@ -144,6 +145,7 @@ export class WebSocketManager {
144145
this._queue.clear();
145146
if (this._shutdown) {
146147
console.log('WebSocket closed (server stopped)');
148+
this._stopLivenessMonitor(); // no socket will reopen
147149
return; // don't reconnect after intentional shutdown
148150
}
149151
console.log('WebSocket closed, reconnecting...');
@@ -169,7 +171,14 @@ export class WebSocketManager {
169171
// It still counts as liveness (above) but frees no in-flight slot.
170172
if (id === 0 && type === 0) {
171173
const payload = data.slice(8);
172-
const msg = JSON.parse(new TextDecoder().decode(payload));
174+
let msg;
175+
try {
176+
msg = JSON.parse(new TextDecoder().decode(payload));
177+
} catch (e) {
178+
// A malformed push must not break the message loop.
179+
console.error('Failed to parse server push JSON:', e);
180+
return;
181+
}
173182
// The server announces the in-flight window sized to its machine.
174183
// Consume it here rather than forwarding to onPush.
175184
if (msg && msg.type === 'config') {
@@ -268,7 +277,11 @@ export class WebSocketManager {
268277
// If still queued it never hit the wire — drop it for free (no server
269278
// work, no buffered bytes). This is the cancellation that actually
270279
// matters; cancelling an already-sent request can't recall the bytes.
271-
if (this._queue.delete(id)) {
280+
// Settle the promise so callers don't leak a forever-pending request.
281+
const queued = this._queue.get(id);
282+
if (queued) {
283+
this._queue.delete(id);
284+
queued.reject(new Error('Request cancelled'));
272285
this.onStatusChange();
273286
return;
274287
}
@@ -287,11 +300,22 @@ export class WebSocketManager {
287300
if (typeof setInterval === 'undefined') {
288301
return;
289302
}
290-
const timer = setInterval(() => this._checkLiveness(),
291-
LIVENESS_INTERVAL_MS);
303+
this._livenessTimer = setInterval(() => this._checkLiveness(),
304+
LIVENESS_INTERVAL_MS);
292305
// Don't keep a Node process alive for this timer (no-op in browsers).
293-
if (timer && typeof timer.unref === 'function') {
294-
timer.unref();
306+
if (this._livenessTimer
307+
&& typeof this._livenessTimer.unref === 'function') {
308+
this._livenessTimer.unref();
309+
}
310+
}
311+
312+
// Stop the liveness timer. Called on intentional shutdown so the interval
313+
// doesn't keep firing _checkLiveness against a socket we'll never reopen.
314+
_stopLivenessMonitor() {
315+
if (this._livenessTimer !== undefined
316+
&& typeof clearInterval !== 'undefined') {
317+
clearInterval(this._livenessTimer);
318+
this._livenessTimer = undefined;
295319
}
296320
}
297321

src/web/test/js/test-websocket-manager.js

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,16 @@ describe('WebSocketManager', () => {
8989
mgr.handleMessage(buildFrame(999, 0, payload));
9090
assert.equal(mgr.pending.size, 0);
9191
});
92+
93+
it('survives a malformed server-push payload', () => {
94+
const mgr = new WebSocketManager('ws://fake');
95+
let pushed = false;
96+
mgr.onPush = () => { pushed = true; };
97+
// id=0 push frame with invalid JSON must not throw or call onPush.
98+
const bad = new TextEncoder().encode('{ not json');
99+
assert.doesNotThrow(() => mgr.handleMessage(buildFrame(0, 0, bad)));
100+
assert.equal(pushed, false, 'no push delivered for malformed JSON');
101+
});
92102
});
93103

94104
describe('request', () => {
@@ -286,6 +296,32 @@ describe('WebSocketManager flow control', () => {
286296
assert.equal(mgr.socket.sent.length, sentBefore,
287297
'a queued cancel sends nothing');
288298
});
299+
300+
it('cancelling a queued request settles its promise', async () => {
301+
const mgr = new WebSocketManager('ws://fake');
302+
await mgr.readyPromise;
303+
304+
// Saturate the wire so the next request stays queued.
305+
const ids = [];
306+
for (let i = 0; i < 200; i++) {
307+
const p = mgr.request({ type: 'tile', n: i });
308+
p.catch(() => {});
309+
ids.push(p.requestId);
310+
}
311+
const queuedId = ids[ids.length - 1];
312+
assert.ok(mgr._queue.has(queuedId), 'last request is queued');
313+
314+
// Re-issue a tracked promise for the queued id so we can observe it.
315+
const entry = mgr._queue.get(queuedId);
316+
const observed = new Promise((resolve, reject) => {
317+
entry.resolve = resolve;
318+
entry.reject = reject;
319+
});
320+
321+
mgr.cancel(queuedId);
322+
// The promise must reject — not hang forever — so callers clean up.
323+
await assert.rejects(observed, { message: 'Request cancelled' });
324+
});
289325
});
290326

291327
describe('WebSocketManager liveness', () => {
@@ -353,6 +389,21 @@ describe('WebSocketManager liveness', () => {
353389
performance.now = realNow;
354390
}
355391
});
392+
393+
it('stops the liveness timer on intentional shutdown', async () => {
394+
const mgr = new WebSocketManager('ws://fake');
395+
await mgr.readyPromise;
396+
assert.notEqual(mgr._livenessTimer, undefined,
397+
'timer runs while connected');
398+
399+
// Server-initiated shutdown: the socket closes and must not reconnect,
400+
// so the recurring liveness check must be cleared too.
401+
mgr._shutdown = true;
402+
mgr.socket.readyState = 3; // CLOSED
403+
mgr.socket.onclose?.();
404+
assert.equal(mgr._livenessTimer, undefined,
405+
'liveness timer cleared after shutdown');
406+
});
356407
});
357408

358409
describe('WebSocketManager.fromCache', () => {

0 commit comments

Comments
 (0)