From 3d53c528c7ec12655f3c2ff5d9ecad6403a6b877 Mon Sep 17 00:00:00 2001 From: Mona Aghili Date: Thu, 27 Aug 2026 13:01:57 +0200 Subject: [PATCH] fix(build): debounce webpack watch mode to avoid mid-write reads (#78) Signed-off-by: Mona Aghili --- build/test/webpack-sdk-factory.test.cjs | 120 ++++++++++++++++++++++++ build/webpack.sdk.factory.mjs | 58 ++++++++++++ 2 files changed, 178 insertions(+) diff --git a/build/test/webpack-sdk-factory.test.cjs b/build/test/webpack-sdk-factory.test.cjs index ee3ef54394..57a82a05e9 100644 --- a/build/test/webpack-sdk-factory.test.cjs +++ b/build/test/webpack-sdk-factory.test.cjs @@ -175,6 +175,126 @@ test('StripBundlePostprocessPlugin: strips webpack\'s bootstrap "use strict" fro } }); +test('resolveWatchAggregateTimeout: falls back to the default when unset', async () => { + const { resolveWatchAggregateTimeout } = await import( + url.pathToFileURL(path.join(__dirname, '..', 'webpack.sdk.factory.mjs')) + ); + assert.equal(resolveWatchAggregateTimeout(undefined), 500); + assert.equal(resolveWatchAggregateTimeout(''), 500); +}); + +test('resolveWatchAggregateTimeout: honors an explicit 0 (no debounce) instead of treating it as unset', async () => { + const { resolveWatchAggregateTimeout } = await import( + url.pathToFileURL(path.join(__dirname, '..', 'webpack.sdk.factory.mjs')) + ); + // `Number(x) || 500` would wrongly collapse "0" back to the default since + // 0 is falsy — this is the regression the helper guards against. + assert.equal(resolveWatchAggregateTimeout('0'), 0); +}); + +test('resolveWatchAggregateTimeout: rejects a non-numeric override instead of silently falling back', async () => { + const { resolveWatchAggregateTimeout } = await import( + url.pathToFileURL(path.join(__dirname, '..', 'webpack.sdk.factory.mjs')) + ); + assert.throws(() => resolveWatchAggregateTimeout('not-a-number'), /must be a non-negative number/); + assert.throws(() => resolveWatchAggregateTimeout('-100'), /must be a non-negative number/); +}); + +test('sdkConfig: never throws on a bad WATCH_AGGREGATE_TIMEOUT — one-shot builds must not fail over a watch-only setting', async () => { + const { sdkConfig } = await import( + url.pathToFileURL(path.join(__dirname, '..', 'webpack.sdk.factory.mjs')) + ); + + const tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'sdk-watch-options-bad-env-test-')); + const prevBuildRoot = process.env.BUILD_ROOT; + const prevCacheDir = process.env.WEBPACK_CACHE_DIR; + const prevTimeout = process.env.WATCH_AGGREGATE_TIMEOUT; + const prevWarn = console.warn; + process.env.BUILD_ROOT = tmpRoot; + process.env.WEBPACK_CACHE_DIR = path.join(tmpRoot, '.webpack-cache'); + process.env.WATCH_AGGREGATE_TIMEOUT = 'not-a-number'; + let warned = false; + console.warn = () => { warned = true; }; + + try { + // sdkConfig() is called for plain `npm run build` too, where + // watchOptions is inert — a stray/typo'd env var must never crash + // that path (see resolveWatchAggregateTimeout's own throwing tests + // above for the input-validation contract itself). + const [minConfig, allConfig] = sdkConfig('word'); + for (const config of [minConfig, allConfig]) { + assert.equal(config.watchOptions.aggregateTimeout, 500, `${config.name}: should fall back to the default on invalid input`); + } + assert.equal(warned, true, 'expected a console.warn about the invalid value'); + } finally { + console.warn = prevWarn; + if (prevBuildRoot === undefined) delete process.env.BUILD_ROOT; else process.env.BUILD_ROOT = prevBuildRoot; + if (prevCacheDir === undefined) delete process.env.WEBPACK_CACHE_DIR; else process.env.WEBPACK_CACHE_DIR = prevCacheDir; + if (prevTimeout === undefined) delete process.env.WATCH_AGGREGATE_TIMEOUT; else process.env.WATCH_AGGREGATE_TIMEOUT = prevTimeout; + fs.rmSync(tmpRoot, { recursive: true, force: true }); + } +}); + +test('sdkConfig: sets a watchOptions.aggregateTimeout above webpack\'s 20ms default on every chunk config', async () => { + const { sdkConfig } = await import( + url.pathToFileURL(path.join(__dirname, '..', 'webpack.sdk.factory.mjs')) + ); + + const tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'sdk-watch-options-test-')); + const prevBuildRoot = process.env.BUILD_ROOT; + const prevCacheDir = process.env.WEBPACK_CACHE_DIR; + process.env.BUILD_ROOT = tmpRoot; + process.env.WEBPACK_CACHE_DIR = path.join(tmpRoot, '.webpack-cache'); + + try { + const [minConfig, allConfig] = sdkConfig('word'); + + // webpack 5's own default (see node_modules/webpack/lib/Watching.js) is + // 20ms — a race window too short to survive a multi-event editor save + // (see issue #78). Both chunk configs must opt into a longer debounce; + // watchOptions is a no-op for one-shot (non --watch) builds, so setting + // it unconditionally is safe for production/CI builds too. + for (const config of [minConfig, allConfig]) { + assert.ok(config.watchOptions, `${config.name}: missing watchOptions`); + assert.equal(config.watchOptions.aggregateTimeout, 500, `${config.name}: default aggregateTimeout should be 500`); + assert.ok( + config.watchOptions.aggregateTimeout > 20, + `${config.name}: aggregateTimeout (${config.watchOptions.aggregateTimeout}) must exceed webpack's 20ms default` + ); + } + } finally { + if (prevBuildRoot === undefined) delete process.env.BUILD_ROOT; else process.env.BUILD_ROOT = prevBuildRoot; + if (prevCacheDir === undefined) delete process.env.WEBPACK_CACHE_DIR; else process.env.WEBPACK_CACHE_DIR = prevCacheDir; + fs.rmSync(tmpRoot, { recursive: true, force: true }); + } +}); + +test('sdkConfig: WATCH_AGGREGATE_TIMEOUT overrides the default watch debounce', async () => { + const { sdkConfig } = await import( + url.pathToFileURL(path.join(__dirname, '..', 'webpack.sdk.factory.mjs')) + ); + + const tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'sdk-watch-options-override-test-')); + const prevBuildRoot = process.env.BUILD_ROOT; + const prevCacheDir = process.env.WEBPACK_CACHE_DIR; + const prevTimeout = process.env.WATCH_AGGREGATE_TIMEOUT; + process.env.BUILD_ROOT = tmpRoot; + process.env.WEBPACK_CACHE_DIR = path.join(tmpRoot, '.webpack-cache'); + process.env.WATCH_AGGREGATE_TIMEOUT = '1200'; + + try { + const [minConfig, allConfig] = sdkConfig('word'); + for (const config of [minConfig, allConfig]) { + assert.equal(config.watchOptions.aggregateTimeout, 1200, `${config.name}: WATCH_AGGREGATE_TIMEOUT override not applied`); + } + } finally { + if (prevBuildRoot === undefined) delete process.env.BUILD_ROOT; else process.env.BUILD_ROOT = prevBuildRoot; + if (prevCacheDir === undefined) delete process.env.WEBPACK_CACHE_DIR; else process.env.WEBPACK_CACHE_DIR = prevCacheDir; + if (prevTimeout === undefined) delete process.env.WATCH_AGGREGATE_TIMEOUT; else process.env.WATCH_AGGREGATE_TIMEOUT = prevTimeout; + fs.rmSync(tmpRoot, { recursive: true, force: true }); + } +}); + test('StripBundlePostprocessPlugin: strips the @@license-banner@@ sentinel after Terser has used it to keep the banner', async () => { const { StripBundlePostprocessPlugin } = await import( url.pathToFileURL(path.join(__dirname, '..', 'webpack.sdk.factory.mjs')) diff --git a/build/webpack.sdk.factory.mjs b/build/webpack.sdk.factory.mjs index 9df3aa4987..10e47bf0fc 100644 --- a/build/webpack.sdk.factory.mjs +++ b/build/webpack.sdk.factory.mjs @@ -29,6 +29,9 @@ * SDK_SOURCE_MAPS '1' to emit source maps for a production build too * (development builds always get them regardless) * WEBPACK_CACHE_DIR override filesystem cache location; defaults to build/.webpack-cache + * WATCH_AGGREGATE_TIMEOUT + * override watch-mode debounce (ms); defaults to 500, + * only takes effect under `--watch` */ import webpack from 'webpack'; @@ -133,6 +136,20 @@ export class StripBundlePostprocessPlugin { } } +// `Number(x) || 500` would silently treat WATCH_AGGREGATE_TIMEOUT=0 (a valid, +// if unusual, "no debounce" choice) the same as an unset/typo'd value, and a +// non-numeric typo would silently fall back to the default instead of +// surfacing the mistake. Exported for the unit test below. +export function resolveWatchAggregateTimeout(rawValue, fallback = 500) { + if (rawValue === undefined || rawValue === '') return fallback; + + const parsed = Number(rawValue); + if (!Number.isFinite(parsed) || parsed < 0) { + throw new Error(`WATCH_AGGREGATE_TIMEOUT must be a non-negative number, got: ${JSON.stringify(rawValue)}`); + } + return parsed; +} + /** * @param {string} moduleName 'word' | 'cell' | 'slide' | 'visio' * @returns {object[]} Two webpack compiler configs: [sdk-all-min, sdk-all] @@ -167,6 +184,22 @@ export function sdkConfig(moduleName) { const appCopyright = process.env.APP_COPYRIGHT || defaultAppCopyright(); const publisherUrl = process.env.PUBLISHER_URL || DEFAULT_PUBLISHER_URL; + // watchOptions is a documented no-op for one-shot builds (see chunkConfig() + // below), but sdkConfig() has no way to know at this point whether the + // caller is about to run under `--watch` or not — so a bad + // WATCH_AGGREGATE_TIMEOUT must never hard-fail a plain `npm run build` + // over a setting that build doesn't even use. Warn and fall back instead + // of propagating resolveWatchAggregateTimeout()'s throw (which stays a + // throw for its own unit tests, where the bad-input contract is exactly + // what's being verified). + let watchAggregateTimeout; + try { + watchAggregateTimeout = resolveWatchAggregateTimeout(process.env.WATCH_AGGREGATE_TIMEOUT); + } catch (err) { + console.warn(`sdkConfig: ${err.message} — falling back to the default (500ms)`); + watchAggregateTimeout = 500; + } + // Sentinel deliberately kept in (stripSentinel: false, the default) — Terser's // format.comments regex below matches on it, and StripBundlePostprocessPlugin // strips it from the asset only after that Terser pass runs. @@ -381,6 +414,31 @@ export function sdkConfig(moduleName) { }, devtool: emitSourceMaps ? 'source-map' : false, + + // Only takes effect under `--watch` (npm run watch:*); ignored for + // one-shot builds. webpack 5's default aggregateTimeout is 20ms + // (see Watching.js) — too short for an editor's save to fully land + // on disk before the loader re-reads it. VS Code (and other + // editors) can flush a single save as more than one MODIFY event, + // especially over a 9p/WSL2 filesystem boundary; the first event + // triggers a rebuild while sdk-concat-loader is re-concatenating + // ~400+ source files, and it can catch this file's write + // mid-flight, truncated. That produces the intermittent tiny + // sdk-all.js / parse-error rebuild described in issue #78, always + // followed by a second, correct rebuild once the write settles. + // + // This only debounces the *trigger* — sdk-concat.cjs still does a + // plain readFile with no size/mtime stability check once the + // timeout fires, so it reduces the race window (20ms -> 500ms of + // required quiescence) rather than closing it structurally. A + // write that's still landing after 500ms of silence (slow disk, + // network mount) could still be read mid-flight. Overridable via + // WATCH_AGGREGATE_TIMEOUT, matching this file's other env-tunable + // knobs, in case 500ms proves insufficient on a given machine/CI + // watcher. + watchOptions: { + aggregateTimeout: watchAggregateTimeout, + }, }; }