fix(server): Sequence destroy() after native teardown

packages/server/lib/serve/Supervisor.js

@@ -546,10 +546,9 @@ class Supervisor extends EventEmitter {
 	 * Stops the server: closes live-reload, the HTTP socket, and the current BuildServer. Teardown
 	 * is tolerant: the socket is closed even if the BuildServer's destroy rejects.
 	 *
-	 * @param {Function} [callback] Invoked once the HTTP server has closed
 	 * @returns {Promise<void>} Resolves once teardown completes
 	 */
-	async destroy(callback) {
+	async destroy() {
 		// Move to the terminal state synchronously, before the first await, so an in-flight #swap or a
 		// late definitionChanged sees DESTROYED at its next guard and adopts nothing.
 		this.#setState(STATE.DESTROYED);
@@ -560,8 +559,14 @@ class Supervisor extends EventEmitter {
 		this.#definitionWatcher = null;
 		this.#liveReloadHandle?.close();
 		this.#detachRelay();
-		this.#httpServer?.close(callback);
 		this.#clearRecoveryTimer();
+		const httpClosed = new Promise((resolve) => {
+			if (!this.#httpServer) {
+				resolve();
+				return;
+			}
+			this.#httpServer.close(() => resolve());
+		});
 		try {
 			await definitionWatcher?.destroy();
 		} catch (err) {
@@ -572,6 +577,7 @@ class Supervisor extends EventEmitter {
 		} catch (err) {
 			log.verbose(`Error while destroying BuildServer: ${err?.message ?? err}`);
 		}
+		await httpClosed;
 	}
 }
 

packages/server/lib/server.js

@@ -20,6 +20,36 @@ const log = getLogger("server");
  * @property {string[]} [ignorePaths=["test-resources/sap/ui/qunit/testrunner.html"]]
  */
 
+/**
+ * Stops a running server.
+ *
+ * Can be awaited or used with a callback. Called without arguments, it returns a
+ * <code>Promise</code> that resolves once teardown completes and rejects if teardown threw.
+ * Called with a callback, it returns <code>undefined</code> and invokes the callback once
+ * teardown completes, with no arguments on success or with the error as its first argument
+ * if teardown threw.
+ *
+ * @public
+ * @callback module:@ui5/server~closeServer
+ * @param {Function} [callback] Invoked once teardown completes. Receives the teardown error as
+ * 						its first argument if teardown threw, otherwise no arguments.
+ * @returns {Promise<void>|undefined} A <code>Promise</code> that resolves once teardown completes
+ * 						when called without a callback, otherwise <code>undefined</code>.
+ */
+
+/**
+ * Handle of a running server instance.
+ *
+ * @public
+ * @typedef {object} module:@ui5/server~ServerInstance
+ * @property {number} port Port the server is listening on
+ * @property {boolean} h2 Whether HTTP/2 is used
+ * @property {module:@ui5/server~closeServer} close Stops the server
+ * @property {Function} reinitialize Re-creates the serving stack. Returns a <code>Promise</code>
+ * 						that resolves once the new stack is in place. A no-op when no
+ * 						<code>graphFactory</code> was provided to {@link module:@ui5/server.serve}.
+ */
+
 
 /**
  * Start a server for the given project (sub-)tree.
@@ -67,10 +97,7 @@ const log = getLogger("server");
  * 										interface and does not depend on @ui5/project, so the owner (the UI5 CLI)
  * 										threads this in to provide the live re-resolution capability. Required
  * 										alongside <code>graphFactory</code>; omit both for a static serve.
- * @returns {Promise<object>} Promise resolving once the server is listening.
- * 							It resolves with an object containing the <code>port</code>,
- * 							<code>h2</code>-flag, a <code>close</code> function to stop the server,
- * 							and a <code>reinitialize</code> function to re-create the serving stack.
+ * @returns {Promise<module:@ui5/server~ServerInstance>} Promise resolving once the server is listening
  */
 export async function serve(graph, {
 	port, changePortIfInUse = false, h2 = false, key, cert,
@@ -108,7 +135,12 @@ export async function serve(graph, {
 		h2,
 		port: supervisor.getPort(),
 		close: function(callback) {
-			supervisor.destroy(callback);
+			const p = supervisor.destroy();
+			if (callback) {
+				p.then(callback, callback);
+			} else {
+				return p;
+			}
 		},
 		reinitialize: function() {
 			return supervisor.reinitialize();

packages/server/test/lib/server/serve/Supervisor.js

@@ -397,7 +397,7 @@ test("destroy() closes live-reload, the socket, and the BuildServer; reinitializ
 
 	const supervisor = await Supervisor.create({}, baseConfig, undefined, graphFactory);
 
-	await new Promise((resolve) => supervisor.destroy(resolve));
+	await supervisor.destroy();
 
 	t.true(liveReloadHandle.close.calledOnce);
 	t.true(httpServer.close.calledOnce);
@@ -415,7 +415,7 @@ test("destroy() closes the socket even when BuildServer.destroy() rejects", asyn
 
 	const supervisor = await Supervisor.create({}, baseConfig, undefined, undefined);
 
-	await new Promise((resolve) => supervisor.destroy(resolve));
+	await supervisor.destroy();
 	t.true(httpServer.close.calledOnce, "socket is closed despite the BuildServer destroy rejection");
 });
 
@@ -1063,6 +1063,6 @@ test("destroy() tears the definition watcher down", async (t) => {
 
 	const supervisor = await Supervisor.create({}, baseConfig, undefined, graphFactory);
 
-	await new Promise((resolve) => supervisor.destroy(resolve));
+	await supervisor.destroy();
 	t.true(definitionWatchers[0].destroy.calledOnce, "watcher destroyed on teardown");
 });

packages/server/test/lib/server/server.js

@@ -10,7 +10,7 @@ import esmock from "esmock";
 function createSupervisorMock({port = 3000, createRejects = null} = {}) {
 	const supervisor = {
 		getPort: sinon.stub().returns(port),
-		destroy: sinon.stub().callsFake((cb) => cb && cb()),
+		destroy: sinon.stub().resolves(),
 		reinitialize: sinon.stub().resolves(),
 	};
 	const create = createRejects ?
@@ -78,6 +78,40 @@ test("serve() close() forwards to supervisor.destroy()", async (t) => {
 	t.true(supervisor.destroy.calledOnce);
 });
 
+test("serve() close() passes the error to the callback when destroy rejects", async (t) => {
+	const {supervisor, Supervisor} = createSupervisorMock();
+	const destroyError = new Error("teardown failed");
+	supervisor.destroy = sinon.stub().rejects(destroyError);
+	const {serve} = await importServe(Supervisor);
+
+	const result = await serve({}, {port: 3000}, undefined);
+	const err = await new Promise((resolve) => result.close(resolve));
+
+	t.is(err, destroyError, "the destroy rejection is forwarded to the close callback");
+});
+
+test("serve() close() returns the destroy promise when no callback is passed", async (t) => {
+	const {supervisor, Supervisor} = createSupervisorMock();
+	const {serve} = await importServe(Supervisor);
+
+	const result = await serve({}, {port: 3000}, undefined);
+	await result.close();
+
+	t.true(supervisor.destroy.calledOnce);
+});
+
+test("serve() close() rejects the returned promise when destroy rejects", async (t) => {
+	const {supervisor, Supervisor} = createSupervisorMock();
+	const destroyError = new Error("teardown failed");
+	supervisor.destroy = sinon.stub().rejects(destroyError);
+	const {serve} = await importServe(Supervisor);
+
+	const result = await serve({}, {port: 3000}, undefined);
+	const err = await t.throwsAsync(result.close());
+
+	t.is(err, destroyError, "the destroy rejection surfaces on the returned promise");
+});
+
 test("serve() rejects when Supervisor.create rejects", async (t) => {
 	const createError = new Error("bind failed");
 	const {Supervisor} = createSupervisorMock({createRejects: createError});