Await watch shutdown and clean up failed startup

index.js

@@ -115,6 +115,10 @@ export class DomStack {
   #pageLayoutNameMap = new Map()
   /** @type {WatchDependencyState | null} dependency observations from the last successful page build */
   #watchDependencies = null
+  #starting = false
+  #acceptWatchEvents = false
+  /** @type {Promise<void> | null} */
+  #stopping = null
 
   // Serialized lock so concurrent chokidar events don't pile up
   /** @type {Promise<void>} */
@@ -174,8 +178,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<void>) | undefined }} params */
+  async #startWatch ({ serve, onInitialBuild }) {
     // ── Initial build (inline, not via builder()) ────────────────────────
     const siteData = await identifyPages(this.#src, this.opts)
 
@@ -235,10 +255,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,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,
@@ -301,6 +323,7 @@ export class DomStack {
       })
     }
 
+    this.#acceptWatchEvents = true
     const enqueue = (/** @type {() => Promise<unknown>} */ fn) => {
       this.#enqueueBuild(fn)
     }
@@ -585,7 +608,9 @@ ${siteData.errors.map(err => ` ${err.message}`).join('\n')}`)
    * @param {() => Promise<unknown>} fn
    */
   #enqueueBuild (fn) {
+    if (!this.#acceptWatchEvents) return
     this.#buildLock = this.#buildLock.then(async () => {
+      if (!this.#acceptWatchEvents) return
       try {
         await fn()
       } catch (err) {
@@ -881,19 +906,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')
   }
 
   /**

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')
     },
   }
 }

test-cases/watch-lifecycle/index.test.js

@@ -0,0 +1,168 @@
+/**
+ * @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<typeof watch>} 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('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 })
+  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')
+})