From 16fee6d3aa4afe6bcd085a55889cbe6c28bd9f92 Mon Sep 17 00:00:00 2001 From: Bret Comnes Date: Sat, 5 Sep 2026 21:08:43 -0700 Subject: [PATCH 1/2] Await watch shutdown and clean up failed startup --- index.js | 69 ++++++++-- lib/build-esbuild/index.js | 70 +++++----- test-cases/watch-lifecycle/index.test.js | 155 +++++++++++++++++++++++ test-cases/watch/index.test.js | 5 +- 4 files changed, 250 insertions(+), 49 deletions(-) create mode 100644 test-cases/watch-lifecycle/index.test.js diff --git a/index.js b/index.js index 69eef4b..3590750 100644 --- a/index.js +++ b/index.js @@ -111,6 +111,10 @@ export class DomStack { #pagesFileOutputMap = new Map() /** @type {Map>} *.pages.* filepath → layouts used by its generated pages */ #pagesFileLayoutMap = new Map() + #starting = false + #acceptWatchEvents = false + /** @type {Promise | null} */ + #stopping = null // Serialized lock so concurrent chokidar events don't pile up /** @type {Promise} */ @@ -170,8 +174,24 @@ export class DomStack { } = { serve: true, }) { - if (this.watching) throw new Error('Already watching.') + if (this.watching || this.#starting || this.#stopping) throw new Error('Already watching.') + this.#starting = true + try { + return await this.#startWatch({ serve, onInitialBuild }) + } catch (error) { + try { + await this.#disposeWatchResources() + } catch (cleanupError) { + throw new AggregateError([error, cleanupError], 'Watch startup and cleanup failed') + } + throw error + } finally { + this.#starting = false + } + } + /** @param {{ serve: boolean, onInitialBuild: ((results: Results) => void | Promise) | undefined }} params */ + async #startWatch ({ serve, onInitialBuild }) { // ── Initial build (inline, not via builder()) ──────────────────────── const siteData = await identifyPages(this.#src, this.opts) @@ -226,10 +246,9 @@ export class DomStack { // ── Copy watchers & dev server ─────────────────────────────────────── const copyDirs = getCopyDirs(this.opts.copy ?? []) - this.#cpxWatchers = [ - cpxWatch(getCopyGlob(this.#src), this.#dest, { ignore: this.opts.ignore ?? [] }), - ...copyDirs.map(copyDir => cpxWatch(copyDir, this.#dest)) - ] + this.#cpxWatchers = [] + this.#cpxWatchers.push(cpxWatch(getCopyGlob(this.#src), this.#dest, { ignore: this.opts.ignore ?? [] })) + for (const copyDir of copyDirs) this.#cpxWatchers.push(cpxWatch(copyDir, this.#dest)) const copyWatchersReady = this.#cpxWatchers.map(async w => { w.on('copy', (/** @type{{ srcPath: string, dstPath: string }} */e) => { @@ -292,6 +311,7 @@ export class DomStack { }) } + this.#acceptWatchEvents = true const enqueue = (/** @type {() => Promise} */ fn) => { this.#enqueueBuild(fn) } @@ -569,7 +589,9 @@ ${siteData.errors.map(err => ` ${err.message}`).join('\n')}`) * @param {() => Promise} fn */ #enqueueBuild (fn) { + if (!this.#acceptWatchEvents) return this.#buildLock = this.#buildLock.then(async () => { + if (!this.#acceptWatchEvents) return try { await fn() } catch (err) { @@ -861,19 +883,38 @@ ${siteData.errors.map(err => ` ${err.message}`).join('\n')}`) } async stopWatching () { + if (this.#stopping) return this.#stopping if ((!this.watching || !this.#cpxWatchers)) throw new Error('Not watching') - if (this.#watcher) this.#watcher.close() - this.#cpxWatchers.forEach(w => { - w.close() - }) + this.#stopping = this.#disposeWatchResources() + try { + await this.#stopping + } finally { + this.#stopping = null + } + } + + async #disposeWatchResources () { + this.#acceptWatchEvents = false + const closures = [ + () => this.#watcher?.close(), + ...(this.#cpxWatchers ?? []).map(w => () => w.close()), + ] + const results = await Promise.allSettled(closures.map(close => Promise.resolve().then(close))) + + // Queued events are cancelled; a build already running may still replace the + // esbuild context, so wait before disposing the final context and server. + await this.#buildLock + results.push(...await Promise.allSettled([ + Promise.resolve().then(() => this.#esbuildContext?.dispose()), + Promise.resolve().then(() => this.#syncServer?.exit()), + ])) this.#watcher = null this.#cpxWatchers = null - if (this.#esbuildContext) { - await this.#esbuildContext.dispose() - this.#esbuildContext = null - } - await this.#syncServer?.exit() + this.#esbuildContext = null this.#syncServer = null + this.#siteData = null + const errors = results.filter(result => result.status === 'rejected').map(result => result.reason) + if (errors.length > 0) throw new AggregateError(errors, 'Watch cleanup failed') } /** diff --git a/lib/build-esbuild/index.js b/lib/build-esbuild/index.js index 653fad5..30305b3 100644 --- a/lib/build-esbuild/index.js +++ b/lib/build-esbuild/index.js @@ -525,40 +525,46 @@ export async function buildEsbuildWatch (src, dest, siteData, opts, watchOpts = const initialResult = browserWatch.initialResult - await writeMetafile({ dest, result: initialResult, shouldWrite: opts?.metafile !== false }) - const outputMap = applyBuildOutputMap({ dest, result: initialResult, siteData, src }) - /** @type {esbuild.BuildContext[]} */ const contexts = [browserWatch.context] + try { + await writeMetafile({ dest, result: initialResult, shouldWrite: opts?.metafile !== false }) + const outputMap = applyBuildOutputMap({ dest, result: initialResult, siteData, src }) - if (siteData.serviceWorker) { - // Keep service-worker-only defines and no-policy watch cleanup behavior out of browser bundles. - const serviceWorkerBuildOpts = createServiceWorkerBuildOpts({ - buildOpts: extendedBuildOpts, - defines: {}, - serviceWorker: siteData.serviceWorker, - src, - }) - const serviceWorkerWatch = await createWatchBuild({ - buildOpts: serviceWorkerBuildOpts, - dest, - label: 'Service worker', - shouldWriteMetafile: false, - }) - applyBuildOutputMap({ - dest, - result: serviceWorkerWatch.initialResult, - siteData, - src, - }) - contexts.push(serviceWorkerWatch.context) - } + if (siteData.serviceWorker) { + // Keep service-worker-only defines and no-policy watch cleanup behavior out of browser bundles. + const serviceWorkerBuildOpts = createServiceWorkerBuildOpts({ + buildOpts: extendedBuildOpts, + defines: {}, + serviceWorker: siteData.serviceWorker, + src, + }) + const serviceWorkerWatch = await createWatchBuild({ + buildOpts: serviceWorkerBuildOpts, + dest, + label: 'Service worker', + shouldWriteMetafile: false, + }) + contexts.push(serviceWorkerWatch.context) + applyBuildOutputMap({ + dest, + result: serviceWorkerWatch.initialResult, + siteData, + src, + }) + } - return { - context: createDisposableBuildContext(contexts), - outputMap, - buildResults: initialResult, - buildOpts: extendedBuildOpts, + return { + context: createDisposableBuildContext(contexts), + outputMap, + buildResults: initialResult, + buildOpts: extendedBuildOpts, + } + } catch (error) { + const cleanup = await Promise.allSettled(contexts.map(context => context.dispose())) + const failures = cleanup.filter(result => result.status === 'rejected').map(result => result.reason) + if (failures.length) throw new AggregateError([error, ...failures], 'Esbuild watch startup and cleanup failed') + throw error } } @@ -620,7 +626,9 @@ async function createWatchBuild ({ buildOpts, dest, label, onEnd, shouldWriteMet function createDisposableBuildContext (contexts) { return { async dispose () { - await Promise.all(contexts.map(context => context.dispose())) + const results = await Promise.allSettled(contexts.map(context => context.dispose())) + const errors = results.filter(result => result.status === 'rejected').map(result => result.reason) + if (errors.length) throw new AggregateError(errors, 'Esbuild watch cleanup failed') }, } } diff --git a/test-cases/watch-lifecycle/index.test.js b/test-cases/watch-lifecycle/index.test.js new file mode 100644 index 0000000..40d06f1 --- /dev/null +++ b/test-cases/watch-lifecycle/index.test.js @@ -0,0 +1,155 @@ +/** + * @import { TestContext } from 'node:test' + * @import { FSWatcher } from 'chokidar' + */ +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { mkdtemp, mkdir, writeFile, readFile, rm, access } from 'node:fs/promises' +import { join } from 'node:path' +import { setTimeout as delay } from 'node:timers/promises' +import chokidar from 'chokidar' +import { DomStack } from '../../index.js' + +/** @param {TestContext} t */ +async function fixture (t) { + const root = await mkdtemp(join(import.meta.dirname, '.tmp-')) + const src = join(root, 'src') + const dest = join(root, 'public') + await mkdir(src) + await writeFile(join(src, 'root.layout.js'), 'export default ({ children }) => children\n') + await writeFile(join(src, 'page.js'), "export default () => 'initial'\n") + const dom = new DomStack(src, dest) + /** @type {FSWatcher | undefined} */ + let watcher + const watch = chokidar.watch + /** @param {Parameters} args */ + const captureWatcher = (...args) => { + watcher = watch(...args) + return watcher + } + t.mock.method(chokidar, 'watch', captureWatcher) + t.after(async () => { + if (dom.watching) await dom.stopWatching() + await rm(root, { recursive: true, force: true }) + }) + return { + dom, + src, + dest, + watcher: () => { + assert.ok(watcher) + return watcher + } + } +} + +test('shutdown awaits asynchronous watcher closure and supports repeated cycles', async t => { + const site = await fixture(t) + await site.dom.watch({ serve: false }) + const watcher = site.watcher() + const close = watcher.close.bind(watcher) + const gate = Promise.withResolvers() + t.mock.method(watcher, 'close', async () => { + await close() + await gate.promise + }) + let stopped = false + const shutdown = site.dom.stopWatching().then(() => { stopped = true }) + await delay(20) + assert.equal(stopped, false) + await assert.rejects(site.dom.watch({ serve: false }), /Already watching/) + gate.resolve(undefined) + await shutdown + assert.equal(site.dom.watching, false) + for (let i = 0; i < 3; i++) { + await site.dom.watch({ serve: false }) + await site.dom.stopWatching() + assert.equal(site.watcher().closed, true) + } +}) + +test('failed startup closes acquired watchers and permits a retry', async t => { + const site = await fixture(t) + await assert.rejects(site.dom.watch({ + serve: false, + onInitialBuild () { throw new Error('callback failed') }, + }), /callback failed/) + assert.equal(site.watcher().closed, true) + assert.equal(site.dom.watching, false) + await site.dom.watch({ serve: false }) + await site.dom.stopWatching() +}) + +test('cleanup reports close failures after releasing remaining resources', async t => { + const site = await fixture(t) + await site.dom.watch({ serve: false }) + const watcher = site.watcher() + const close = watcher.close.bind(watcher) + t.mock.method(watcher, 'close', async () => { + await close() + throw new Error('close failed') + }) + await assert.rejects(site.dom.stopWatching(), error => { + assert.ok(error instanceof AggregateError) + assert.match(error.errors[0].message, /close failed/) + return true + }) + assert.equal(site.dom.watching, false) + await site.dom.watch({ serve: false }) + await site.dom.stopWatching() +}) + +test('service-worker startup failure disposes both esbuild contexts', async t => { + const site = await fixture(t) + await writeFile(join(site.src, 'client.js'), 'console.log("client")\n') + await writeFile(join(site.src, 'service-worker.js'), 'export default (\n') + await writeFile(join(site.src, 'esbuild.settings.js'), `import { appendFileSync } from 'node:fs' +export default options => ({ ...options, plugins: [{ + name: 'observe-disposal', + setup (build) { + build.onDispose(() => appendFileSync(import.meta.dirname + '/.disposed', 'disposed\\n')) + }, +}] }) +`) + await assert.rejects(site.dom.watch({ serve: false }), /Error starting esbuild watch context/) + for (let i = 0; i < 100; i++) { + const contents = await readFile(join(site.src, '.disposed'), 'utf8').catch(() => '') + if (contents === 'disposed\ndisposed\n') break + await delay(10) + } + assert.equal(await readFile(join(site.src, '.disposed'), 'utf8'), 'disposed\ndisposed\n') + assert.equal(site.dom.watching, false) +}) + +test('shutdown drains an active page build and cancels queued events', { timeout: 15_000 }, async t => { + const site = await fixture(t) + await writeFile(join(site.src, 'page.js'), `import { existsSync, writeFileSync, appendFileSync } from 'node:fs' +import { setTimeout } from 'node:timers/promises' +export default async () => { + const root = import.meta.dirname + if (existsSync(root + '/.block')) { + appendFileSync(root + '/.runs', 'run\\n') + writeFileSync(root + '/.started', '') + while (!existsSync(root + '/.release')) await setTimeout(10) + } + return 'rendered' +} +`) + await site.dom.watch({ serve: false }) + await writeFile(join(site.src, '.block'), '') + site.watcher().emit('change', join(site.src, 'page.js')) + for (let i = 0; i < 500; i++) { + if (await access(join(site.src, '.started')).then(() => true, () => false)) break + await delay(10) + } + await access(join(site.src, '.started')) + site.watcher().emit('change', join(site.src, 'page.js')) + let stopped = false + const shutdown = site.dom.stopWatching().then(() => { stopped = true }) + await delay(20) + assert.equal(stopped, false) + await writeFile(join(site.src, '.release'), '') + await shutdown + assert.equal(await readFile(join(site.src, '.runs'), 'utf8'), 'run\n') + assert.equal(await readFile(join(site.dest, 'index.html'), 'utf8'), 'rendered') +}) diff --git a/test-cases/watch/index.test.js b/test-cases/watch/index.test.js index e39616f..a917d50 100644 --- a/test-cases/watch/index.test.js +++ b/test-cases/watch/index.test.js @@ -122,10 +122,6 @@ test.describe('watch', () => { test('progressive rebuilds', { timeout: 60_000 }, async (t) => { const { src, dest, tmp } = await setupTempSite() - t.after(async () => { - await rm(tmp, { recursive: true, force: true }) - }) - const mockLog = mock.method(console, 'log') const loggerLogs = /** @type {string[]} */ ([]) const logger = createTestLogger(loggerLogs) @@ -134,6 +130,7 @@ test.describe('watch', () => { t.after(async () => { if (domStack.watching) await domStack.stopWatching() mockLog.mock.restore() + await rm(tmp, { recursive: true, force: true }) }) // ── Initial build ──────────────────────────────────────────────── From 74d3cbf20a8f55ae67257f37a96e67c6dba6eb8f Mon Sep 17 00:00:00 2001 From: Bret Comnes Date: Sat, 5 Sep 2026 21:26:08 -0700 Subject: [PATCH 2/2] Honor shutdown from the initial build callback --- index.js | 3 +++ test-cases/watch-lifecycle/index.test.js | 13 +++++++++++++ 2 files changed, 16 insertions(+) diff --git a/index.js b/index.js index 3590750..0341797 100644 --- a/index.js +++ b/index.js @@ -302,6 +302,9 @@ export class DomStack { await onInitialBuild?.(report) + // The callback may have stopped this watch session. + if (!this.watching || this.#stopping) return report + if (serve) { this.#syncServer = await createServer({ server: this.#dest, diff --git a/test-cases/watch-lifecycle/index.test.js b/test-cases/watch-lifecycle/index.test.js index 40d06f1..03e8445 100644 --- a/test-cases/watch-lifecycle/index.test.js +++ b/test-cases/watch-lifecycle/index.test.js @@ -80,6 +80,19 @@ test('failed startup closes acquired watchers and permits a retry', async t => { await site.dom.stopWatching() }) +test('stopping in the initial-build callback does not resume watch startup', async t => { + const site = await fixture(t) + await site.dom.watch({ + serve: false, + onInitialBuild: () => site.dom.stopWatching(), + }) + assert.equal(site.dom.watching, false) + assert.equal(site.watcher().closed, true) + assert.equal(site.watcher().listenerCount('change'), 0) + await site.dom.watch({ serve: false }) + await site.dom.stopWatching() +}) + test('cleanup reports close failures after releasing remaining resources', async t => { const site = await fixture(t) await site.dom.watch({ serve: false })