From f0661a9d8e9f6832bf0c9c1bf33be84990e3c607 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 16 May 2026 07:42:34 +0100 Subject: [PATCH] fix(engine-flush-defer): address Qodo review Two issues from the initial review: 1. Install guard blocked retries. Setting `installed = true` BEFORE requiring + validating engine.io meant a transient require error permanently disabled the patch for the rest of the process. Now the flag is set only after both checks pass; on failure, the warning is logged and a later boot path can retry. 2. engineFlushDefer was undocumented in settings.json.template. Added with the same prose as Settings.ts, including the "wire bytes are unchanged" / "no meaningful wall-clock latency" notes so operators see the safety case. Co-Authored-By: Claude Opus 4.7 (1M context) --- settings.json.template | 13 +++++++++++++ src/node/utils/EngineFlushDeferral.ts | 10 +++++++--- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/settings.json.template b/settings.json.template index e88e82a36..c37bfe7f9 100644 --- a/settings.json.template +++ b/settings.json.template @@ -733,6 +733,19 @@ */ "loadTest": false, + /* + * Defer engine.io socket flush onto the next microtask so multiple + * sendPacket() calls in the same task accumulate in writeBuffer before + * the underlying transport.send drains. Pairs with engine.io's existing + * batched-send path so high-fan-out scenarios produce fewer WebSocket + * frames. Microtask deferral adds no meaningful wall-clock latency — + * microtasks drain before any subsequent macrotask. Wire bytes are + * unchanged. + * + * #7756 / #7767. Default off; production unaffected. + */ + "engineFlushDefer": false, + /** * Disable dump of objects preventing a clean exit */ diff --git a/src/node/utils/EngineFlushDeferral.ts b/src/node/utils/EngineFlushDeferral.ts index e8c15b4ea..36b3804ed 100644 --- a/src/node/utils/EngineFlushDeferral.ts +++ b/src/node/utils/EngineFlushDeferral.ts @@ -47,7 +47,6 @@ const SCHEDULED = Symbol('engineFlushScheduled'); export const installEngineFlushDeferral = (): void => { if (installed) return; - installed = true; let SocketProto: {sendPacket: (...a: unknown[]) => unknown}; try { @@ -55,12 +54,17 @@ export const installEngineFlushDeferral = (): void => { SocketProto = require('engine.io/build/socket').Socket.prototype; } catch (err: any) { logger.warn(`Unable to install engine.io flush deferral (module not found): ${err && err.message || err}`); - return; + return; // Leave `installed` false so a later boot path can retry. } if (typeof SocketProto.sendPacket !== 'function') { logger.warn('engine.io Socket shape unexpected; skipping flush deferral patch'); - return; + return; // Leave `installed` false so a later boot path can retry. } + // Only after both require and shape check succeed do we record that the + // patch is installed. Setting the flag before validation (the original + // code) would have permanently disabled retries after a transient + // require failure in test/CI environments where socket.io may load late. + installed = true; // Re-implementing sendPacket inline rather than wrapping the original // so the single closing `this.flush()` becomes a microtask-coalesced