From 86d6003a75f9194f1f4145f59535e5afb7537670 Mon Sep 17 00:00:00 2001 From: "posthog[bot]" <206114724+posthog[bot]@users.noreply.github.com> Date: Tue, 7 Jul 2026 15:40:32 +0000 Subject: [PATCH] fix(skills): stop skills updater throwing ENOENT on removed staging dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The skills auto-updater staged downloads into a shared `.new` directory and called `readdir` on it with no ENOENT guard. A second workspace-server instance (another window/project) — or the atomic swap `rename` — could remove that shared path mid-run, so `readdir` threw `ENOENT ... scandir '.../skills.new'`, which surfaced as error-tracking noise even though the failure is non-fatal (the updater retries on the next interval and bundled skills keep working). Isolate the staging (and backup) dirs per instance via a stable `runId` (`process.pid`) so a concurrent instance can no longer clobber the path, keeping them on the same filesystem so the atomic-swap `rename` still works. As defense in depth, treat a missing staging dir as an empty/no-op cycle rather than an ENOENT throw at both the validate `readdir` and the swap. Generated-By: PostHog Code Task-Id: 905f1443-bcf1-47a5-b6b8-c7b6cf65ad7e --- .../posthog-plugin/posthog-plugin.test.ts | 51 ++++++++++++++++ .../services/posthog-plugin/posthog-plugin.ts | 6 ++ .../posthog-plugin/update-skills-saga.ts | 59 +++++++++++++++---- 3 files changed, 105 insertions(+), 11 deletions(-) diff --git a/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.test.ts b/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.test.ts index 1904da6288..592805eb85 100644 --- a/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.test.ts +++ b/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")); diff --git a/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.ts b/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.ts index 913d8bac56..33f5f8db8f 100644 --- a/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.ts +++ b/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.ts @@ -205,6 +205,12 @@ export class PosthogPluginService extends TypedEventEmitter 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 + // `.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), diff --git a/packages/workspace-server/src/services/posthog-plugin/update-skills-saga.ts b/packages/workspace-server/src/services/posthog-plugin/update-skills-saga.ts index 9177aa4487..698c18cdb5 100644 --- a/packages/workspace-server/src/services/posthog-plugin/update-skills-saga.ts +++ b/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 + * (`.new-` / `.old-`) 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; @@ -60,7 +68,7 @@ export class UpdateSkillsSaga extends Saga< protected async execute( input: UpdateSkillsInput, ): Promise { - 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 { + 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`. */