fix(skills): stop skills updater throwing ENOENT on removed staging dir

packages/workspace-server/src/services/posthog-plugin/posthog-plugin.test.ts

@@ -486,6 +486,57 @@ describe("PosthogPluginService", () => {
       expect(vol.existsSync(`${RUNTIME_SKILLS_DIR}.new`)).toBe(false);
     });
 
+    it("treats a staging dir removed mid-run as a no-op, not an ENOENT error", async () => {
+      // A previously-downloaded cache exists, so a lost staging dir is benign.
+      vol.mkdirSync(`${RUNTIME_SKILLS_DIR}/cached-skill`, { recursive: true });
+      vol.writeFileSync(
+        `${RUNTIME_SKILLS_DIR}/cached-skill/SKILL.md`,
+        "# Cached",
+      );
+
+      // Simulate a concurrent workspace-server instance (another window/project)
+      // removing this run's staging dir out from under it — the original crash
+      // where the unguarded readdir threw `ENOENT ... scandir '.../skills.new'`.
+      const stagingDir = `${RUNTIME_SKILLS_DIR}.new-${process.pid}`;
+      mockExtractZip.mockImplementation(async () => {
+        if (vol.existsSync(stagingDir)) {
+          vol.rmSync(stagingDir, { recursive: true, force: true });
+        }
+      });
+
+      const handler = vi.fn();
+      service.on("skillsUpdated", handler);
+      await service.updateSkills();
+
+      // No ENOENT surfaced to error tracking, existing cache preserved, and no
+      // false "updated" signal — the cycle is a silent no-op.
+      expect(mockAnalytics.captureException).not.toHaveBeenCalled();
+      expect(handler).not.toHaveBeenCalled();
+      expect(
+        vol.readFileSync(
+          `${RUNTIME_SKILLS_DIR}/cached-skill/SKILL.md`,
+          "utf-8",
+        ),
+      ).toBe("# Cached");
+    });
+
+    it("isolates the staging dir per instance so it never uses the shared path", async () => {
+      setupBundledPlugin();
+      simulateExtractZip();
+
+      await service.updateSkills();
+
+      // The shared `.new` path must never be created; staging is per-process.
+      expect(vol.existsSync(`${RUNTIME_SKILLS_DIR}.new`)).toBe(false);
+      // And this run's isolated staging/backup dirs are cleaned up on success.
+      expect(vol.existsSync(`${RUNTIME_SKILLS_DIR}.new-${process.pid}`)).toBe(
+        false,
+      );
+      expect(vol.existsSync(`${RUNTIME_SKILLS_DIR}.old-${process.pid}`)).toBe(
+        false,
+      );
+    });
+
     it("cleans up temp dir even on error", async () => {
       mockExtractZip.mockRejectedValue(new Error("extraction failed"));
 

packages/workspace-server/src/services/posthog-plugin/posthog-plugin.ts

@@ -205,6 +205,12 @@ export class PosthogPluginService extends TypedEventEmitter<PosthogPluginEvents>
         runtimeSkillsDir: this.runtimeSkillsDir,
         runtimePluginDir: this.runtimePluginDir,
         tempDir,
+        // Isolate this instance's staging dirs from any other running
+        // workspace-server instance (another window/project) so a shared
+        // `<skills>.new` path can't be clobbered mid-run. The pid is stable
+        // across this process's update cycles, so the `create-staging-dir` step
+        // self-heals any leftover staging dir from an interrupted prior run.
+        runId: String(process.pid),
         skillsZipUrl: SKILLS_ZIP_URL,
         contextMillZipUrl: CONTEXT_MILL_ZIP_URL,
         downloadFile: (url, destPath) => this.downloadFile(url, destPath),

packages/workspace-server/src/services/posthog-plugin/update-skills-saga.ts

@@ -42,6 +42,14 @@ export interface UpdateSkillsInput {
   runtimeSkillsDir: string;
   runtimePluginDir: string;
   tempDir: string;
+  /**
+   * Stable, per-instance token used to isolate this run's staging directories
+   * (`<skills>.new-<runId>` / `<skills>.old-<runId>`) from other running
+   * workspace-server instances. A second instance (another window/project)
+   * staging into a shared path could otherwise `rm`/`rename` it out from under
+   * this run, making the validate/swap steps throw a spurious ENOENT.
+   */
+  runId: string;
   skillsZipUrl: string;
   contextMillZipUrl: string;
   downloadFile: (url: string, destPath: string) => Promise<void>;
@@ -60,7 +68,7 @@ export class UpdateSkillsSaga extends Saga<
   protected async execute(
     input: UpdateSkillsInput,
   ): Promise<UpdateSkillsOutput> {
-    const newSkillsDir = `${input.runtimeSkillsDir}.new`;
+    const newSkillsDir = `${input.runtimeSkillsDir}.new-${input.runId}`;
 
     // Step 1: create staging dir
     await this.step({
@@ -108,16 +116,18 @@ export class UpdateSkillsSaga extends Saga<
       }
     });
 
-    // Step 3: validate skills. An empty staging dir means both downloads
-    // produced nothing this cycle (e.g. a transient network failure — the
-    // download steps above are intentionally non-fatal). The existing skills
-    // cache and the bundled skills remain in place, so this is a no-op cycle,
-    // not a failure: skip the swap and try again on the next interval rather
-    // than throwing, which would surface a misleading "no skills" exception.
+    // Step 3: validate skills. An empty (or missing) staging dir means both
+    // downloads produced nothing this cycle (e.g. a transient network failure —
+    // the download steps above are intentionally non-fatal — or the staging dir
+    // was cleaned up by a concurrent instance). The existing skills cache and
+    // the bundled skills remain in place, so this is a no-op cycle, not a
+    // failure: skip the swap and try again on the next interval rather than
+    // throwing, which would surface a misleading "no skills" exception. Reading
+    // a missing dir is treated as empty rather than an ENOENT throw.
     const stagedSkillCount = await this.readOnlyStep(
       "validate-skills",
       async () => {
-        const entries = await readdir(newSkillsDir);
+        const entries = await this.safeReaddir(newSkillsDir);
         return entries.length;
       },
     );
@@ -129,8 +139,7 @@ export class UpdateSkillsSaga extends Saga<
       await rm(newSkillsDir, { recursive: true, force: true });
 
       const hasCachedSkills =
-        existsSync(input.runtimeSkillsDir) &&
-        (await readdir(input.runtimeSkillsDir)).length > 0;
+        (await this.safeReaddir(input.runtimeSkillsDir)).length > 0;
 
       if (hasCachedSkills) {
         // A transient blip (e.g. network failure — the download steps above
@@ -157,8 +166,19 @@ export class UpdateSkillsSaga extends Saga<
       );
     }
 
+    // The staging dir is validated as non-empty above, but a concurrent
+    // instance (or an interrupted prior run) could still have removed it in the
+    // meantime. Guard the swap so a missing staging dir is a no-op cycle rather
+    // than an ENOENT thrown from `rename`.
+    if (!existsSync(newSkillsDir)) {
+      this.log.warn(
+        "Staging skills dir disappeared before swap; skipping this cycle",
+      );
+      return { updated: false };
+    }
+
     // Step 4: atomic swap
-    const oldSkillsDir = `${input.runtimeSkillsDir}.old`;
+    const oldSkillsDir = `${input.runtimeSkillsDir}.old-${input.runId}`;
     await this.step({
       name: "swap-skills-cache",
       execute: async () => {
@@ -202,6 +222,23 @@ export class UpdateSkillsSaga extends Saga<
     return { updated: true };
   }
 
+  /**
+   * Reads a directory's entries, treating a missing directory as empty rather
+   * than throwing ENOENT. The staging dir can be removed out from under a run by
+   * a concurrent workspace-server instance or the atomic swap; a vanished dir
+   * should be handled as a no-op cycle, not surfaced as an error.
+   */
+  private async safeReaddir(dir: string): Promise<string[]> {
+    try {
+      return await readdir(dir);
+    } catch (err) {
+      if ((err as NodeJS.ErrnoException).code === "ENOENT") {
+        return [];
+      }
+      throw err;
+    }
+  }
+
   /**
    * Downloads a skills zip from `url`, extracts it, and merges skill directories into `destDir`.
    */