Refresh the served bb-app artifact after restart

apps/server/src/services/install/bb-app-artifact.ts

@@ -1,5 +1,5 @@
 import { execFile } from "node:child_process";
-import { mkdir, readFile, rename, rm, stat, writeFile } from "node:fs/promises";
+import { mkdir, readFile, rename, rm } from "node:fs/promises";
 import { dirname, join, resolve } from "node:path";
 import { fileURLToPath } from "node:url";
 import { promisify } from "node:util";
@@ -28,13 +28,6 @@ interface BbAppPackageJson {
   version: string;
 }
 
-interface BbAppArtifactMetadata {
-  protocolVersion: number;
-  version: string;
-}
-
-const pendingBuilds = new Map<string, Promise<string>>();
-
 async function defaultCommandRunner(
   command: string,
   args: readonly string[],
@@ -110,14 +103,6 @@ function safeVersionFilePart(version: string): string {
   return version.replace(/[^a-zA-Z0-9._-]/gu, "_");
 }
 
-async function pathIsFile(path: string): Promise<boolean> {
-  try {
-    return (await stat(path)).isFile();
-  } catch {
-    return false;
-  }
-}
-
 export function createBbAppArtifactService(
   options: CreateBbAppArtifactServiceOptions,
 ): BbAppArtifactService {
@@ -127,6 +112,7 @@ export function createBbAppArtifactService(
   const protocolVersion =
     options.protocolVersion ?? HOST_DAEMON_PROTOCOL_VERSION;
   let resolvedPackagePromise: Promise<ResolvedBbAppPackage> | undefined;
+  let artifactPromise: Promise<string> | undefined;
 
   function getResolvedPackage(): Promise<ResolvedBbAppPackage> {
     resolvedPackagePromise ??= resolveBbAppPackage(serverEntryUrl);
@@ -140,35 +126,9 @@ export function createBbAppArtifactService(
       cacheDir,
       `bb-app-${safeVersionFilePart(packageJson.version)}-protocol-${protocolVersion}.tgz`,
     );
-    const metadataPath = `${tarballPath}.json`;
-    let cachedMetadata: BbAppArtifactMetadata | null = null;
-    try {
-      const parsed: unknown = JSON.parse(await readFile(metadataPath, "utf8"));
-      if (
-        parsed !== null &&
-        typeof parsed === "object" &&
-        "version" in parsed &&
-        parsed.version === packageJson.version &&
-        "protocolVersion" in parsed &&
-        parsed.protocolVersion === protocolVersion
-      ) {
-        cachedMetadata = {
-          version: parsed.version,
-          protocolVersion: parsed.protocolVersion,
-        };
-      }
-    } catch {
-      cachedMetadata = null;
-    }
-    if (cachedMetadata && (await pathIsFile(tarballPath))) {
-      return tarballPath;
-    }
-
     await mkdir(cacheDir, { recursive: true });
-    await Promise.all([
-      rm(tarballPath, { force: true }),
-      rm(metadataPath, { force: true }),
-    ]);
+    // Clean up the metadata sidecar older installs persisted; nothing reads it.
+    await rm(`${tarballPath}.json`, { force: true });
 
     if (resolved.layout === "repo") {
       const repoRoot = resolve(packageRoot, "../..");
@@ -188,32 +148,28 @@ export function createBbAppArtifactService(
     if (!packedName) {
       throw new Error("npm pack did not report a tarball name");
     }
+    // Publish by atomic rename rather than deleting first: a failed build then
+    // leaves the previously served artifact in place instead of stranding
+    // daemons with no installable package at all. npm pack writes
+    // `bb-app-<version>.tgz`, which can never collide with the
+    // `-protocol-<n>`-suffixed destination in the same directory.
     const packedPath = join(cacheDir, packedName);
     await rename(packedPath, tarballPath);
-    await writeFile(
-      metadataPath,
-      `${JSON.stringify({ version: packageJson.version, protocolVersion })}\n`,
-      "utf8",
-    );
     return tarballPath;
   }
 
   return {
-    async getTarballPath(): Promise<string> {
-      const version = (await getResolvedPackage()).packageJson.version;
-      const tarballPath = join(
-        cacheDir,
-        `bb-app-${safeVersionFilePart(version)}-protocol-${protocolVersion}.tgz`,
-      );
-      const existing = pendingBuilds.get(tarballPath);
-      if (existing) {
-        return existing;
-      }
-      const build = buildTarball().finally(() => {
-        pendingBuilds.delete(tarballPath);
+    getTarballPath(): Promise<string> {
+      // Build once per process from the package this process is running, so a
+      // restart into a different source build at the same version and protocol
+      // still serves the current bits. Memoizing the promise (assigned
+      // synchronously) collapses concurrent callers onto one build; clearing it
+      // on rejection keeps a transient build failure from being cached forever.
+      artifactPromise ??= buildTarball().catch((error: unknown) => {
+        artifactPromise = undefined;
+        throw error;
       });
-      pendingBuilds.set(tarballPath, build);
-      return build;
+      return artifactPromise;
     },
     async getVersion(): Promise<string> {
       return (await getResolvedPackage()).packageJson.version;

apps/server/test/app/bb-app-artifact.test.ts

@@ -104,6 +104,45 @@ describe.each(MODES)("bb-app artifact service (%s)", (mode) => {
     expect(calls.filter((call) => call.command === "npm")).toHaveLength(1);
   });
 
+  it("rebuilds the artifact after package contents change without a version or protocol change", async () => {
+    const test = await fixture(mode);
+    const calls: Array<{
+      command: string;
+      args: readonly string[];
+      cwd: string;
+    }> = [];
+    const runner: BbAppArtifactCommandRunner = async (command, args, cwd) => {
+      calls.push({ command, args, cwd });
+      if (command === "pnpm") return "built";
+      return (await execFileAsync(command, [...args], { cwd })).stdout;
+    };
+    const options = {
+      dataDir: join(test.root, "data"),
+      commandRunner: runner,
+      serverEntryUrl: pathToFileURL(test.serverEntry).href,
+      protocolVersion: 51,
+    };
+
+    const first = await createBbAppArtifactService(options).getTarballPath();
+    expect(
+      (await execFileAsync("tar", ["-xOzf", first, "package/README.md"]))
+        .stdout,
+    ).toBe("fixture\n");
+
+    await writeFile(join(test.packageRoot, "README.md"), "updated\n");
+    const second = await createBbAppArtifactService(options).getTarballPath();
+
+    expect(second).toBe(first);
+    expect(
+      (await execFileAsync("tar", ["-xOzf", second, "package/README.md"]))
+        .stdout,
+    ).toBe("updated\n");
+    expect(calls.filter((call) => call.command === "npm")).toHaveLength(2);
+    expect(calls.filter((call) => call.command === "pnpm")).toHaveLength(
+      isRepoMode(mode) ? 2 : 0,
+    );
+  });
+
   it("builds a fresh artifact when the protocol changes at the same package version", async () => {
     const test = await fixture(mode);
     const calls: Array<{
@@ -137,4 +176,42 @@ describe.each(MODES)("bb-app artifact service (%s)", (mode) => {
       isRepoMode(mode) ? 2 : 0,
     );
   });
+
+  it("keeps serving the previous artifact when a rebuild fails, and retries after", async () => {
+    const test = await fixture(mode);
+    let failNextPack = false;
+    const runner: BbAppArtifactCommandRunner = async (command, args, cwd) => {
+      if (command === "pnpm") return "built";
+      if (failNextPack) throw new Error("npm pack exploded");
+      return (await execFileAsync(command, [...args], { cwd })).stdout;
+    };
+    const options = {
+      dataDir: join(test.root, "data"),
+      commandRunner: runner,
+      serverEntryUrl: pathToFileURL(test.serverEntry).href,
+      protocolVersion: 51,
+    };
+
+    const tarball = await createBbAppArtifactService(options).getTarballPath();
+
+    // A later process restarts, its rebuild fails, and the artifact it would
+    // have replaced must survive: daemons keep an installable package instead
+    // of a 404 with nothing on disk.
+    failNextPack = true;
+    await writeFile(join(test.packageRoot, "README.md"), "updated\n");
+    const service = createBbAppArtifactService(options);
+    await expect(service.getTarballPath()).rejects.toThrow("npm pack exploded");
+    expect(
+      (await execFileAsync("tar", ["-xOzf", tarball, "package/README.md"]))
+        .stdout,
+    ).toBe("fixture\n");
+
+    // The rejection is not memoized, so the next request rebuilds for real.
+    failNextPack = false;
+    await expect(service.getTarballPath()).resolves.toBe(tarball);
+    expect(
+      (await execFileAsync("tar", ["-xOzf", tarball, "package/README.md"]))
+        .stdout,
+    ).toBe("updated\n");
+  });
 });