From 9e6f5ca9cb1921333fd03335e86e33ae24333d90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=95=85=E7=92=83?= Date: Tue, 1 Sep 2026 16:37:12 +0800 Subject: [PATCH] fix(skill): retry registry fetches and tolerate blocked backup cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - bl skill init: raise index timeout 10sā†’30s and retry transient network failures (3 attempts); advisor sync silent channel stays fail-fast - make post-swap backup deletion best-effort so host safe-delete guards cannot fail a completed install (postinstall.js mirror included) - add unit coverage for guard-blocked cleanup and registry retry policy --- packages/cli/postinstall.js | 14 +- packages/core/src/advisor/sync.ts | 5 +- packages/core/src/skills/extract.ts | 9 +- packages/core/src/skills/installer.ts | 21 ++- packages/core/src/skills/registry.ts | 169 +++++++++++-------- packages/core/tests/skills-installer.test.ts | 100 +++++++++-- 6 files changed, 226 insertions(+), 92 deletions(-) diff --git a/packages/cli/postinstall.js b/packages/cli/postinstall.js index 8dd3f5e3d..bcd928313 100644 --- a/packages/cli/postinstall.js +++ b/packages/cli/postinstall.js @@ -181,7 +181,13 @@ function atomicSwap(tmpDir, catalogDir) { if (existsSync(backup) && !existsSync(catalogDir)) renameSync(backup, catalogDir); throw err; } - if (existsSync(backup)) rmSync(backup, { recursive: true, force: true }); + // Best-effort cleanup (symmetric with core skills/extract.ts): the swap already + // succeeded, so a backup deletion failure must not fail the pre-download + try { + if (existsSync(backup)) rmSync(backup, { recursive: true, force: true }); + } catch { + /* keep the backup on disk rather than report a completed swap as failed */ + } } async function main() { @@ -214,7 +220,11 @@ async function main() { } atomicSwap(tmpDir, catalogDir); } catch (err) { - if (existsSync(tmpDir)) rmSync(tmpDir, { recursive: true, force: true }); + try { + if (existsSync(tmpDir)) rmSync(tmpDir, { recursive: true, force: true }); + } catch { + /* cleanup must not mask the original error */ + } throw err; } diff --git a/packages/core/src/advisor/sync.ts b/packages/core/src/advisor/sync.ts index 45d9cb430..442727924 100644 --- a/packages/core/src/advisor/sync.ts +++ b/packages/core/src/advisor/sync.ts @@ -37,6 +37,8 @@ const MODELS_FILE = "models.jsonl"; const THROTTLE_MS = 12 * 60 * 60 * 1000; // 12h /** Tighter than the interactive default: the silent channel must not stall `bl advisor recommend` */ const INDEX_TIMEOUT_MS = 3000; +/** No retries on the silent channel: a failed sync simply tries again on the next recommend */ +const FETCH_ATTEMPTS = 1; interface SyncState { lastChecked: number; @@ -108,7 +110,7 @@ function wikiLockNeedsBackfill(contentHash: string): boolean { /** Fetch skills/index.json via the shared registry client and extract the wiki skill entry; returns null on any failure */ async function fetchIndexEntry(): Promise { try { - const index = await fetchSkillsIndex(INDEX_TIMEOUT_MS); + const index = await fetchSkillsIndex(INDEX_TIMEOUT_MS, FETCH_ATTEMPTS); return index.skills[WIKI_SKILL_NAME] ?? null; } catch { return null; @@ -160,6 +162,7 @@ export async function maybeSyncWikiData(): Promise { entry, detectInstalledAgents(), previousLinks, + FETCH_ATTEMPTS, ); recordWikiInLock(record.lockEntry); } catch { diff --git a/packages/core/src/skills/extract.ts b/packages/core/src/skills/extract.ts index d8ed82543..85ec2cb46 100644 --- a/packages/core/src/skills/extract.ts +++ b/packages/core/src/skills/extract.ts @@ -100,5 +100,12 @@ export function atomicSwap(tmpDir: string, destDir: string): void { if (existsSync(backup) && !existsSync(destDir)) renameSync(backup, destDir); throw err; } - if (existsSync(backup)) rmSync(backup, { recursive: true, force: true }); + // Best-effort cleanup: the swap already succeeded, so a backup deletion failure + // (permissions, host safe-delete guards on large dirs) must not fail the install; + // leftover .old-* dirs are inert (skill status scans ignore them) + try { + if (existsSync(backup)) rmSync(backup, { recursive: true, force: true }); + } catch { + /* keep the backup on disk rather than report a completed install as failed */ + } } diff --git a/packages/core/src/skills/installer.ts b/packages/core/src/skills/installer.ts index 1f93d1e31..05f1d6c55 100644 --- a/packages/core/src/skills/installer.ts +++ b/packages/core/src/skills/installer.ts @@ -61,12 +61,22 @@ export async function installSkillFromBuffer( atomicSwap(tmpDir, dest); return { name, path: dest, meta }; } finally { - if (existsSync(tmpDir)) rmSync(tmpDir, { recursive: true, force: true }); + // Best-effort cleanup: on failure paths this must not mask the original error, + // and on success the dir is already renamed away (existsSync ā†’ false) + try { + if (existsSync(tmpDir)) rmSync(tmpDir, { recursive: true, force: true }); + } catch { + /* leave the temp dir rather than hide the real error */ + } } } /** Install a single skill by index entry (download + validate + write to disk) */ -export async function installSkill(name: string, entry: SkillIndexEntry): Promise { +export async function installSkill( + name: string, + entry: SkillIndexEntry, + downloadAttempts?: number, +): Promise { if (entry.compression && entry.compression !== "tar.br") { throw new BailianError( `Skill ${name} uses unsupported compression format: ${entry.compression}`, @@ -74,7 +84,7 @@ export async function installSkill(name: string, entry: SkillIndexEntry): Promis "Upgrade bailian-cli to the latest version and retry", ); } - const buffer = await downloadSkillAsset(name, entry); + const buffer = await downloadSkillAsset(name, entry, downloadAttempts); return installSkillFromBuffer(name, buffer, entry.contentHash); } @@ -116,14 +126,17 @@ export interface SkillInstallRecord { * entry (batch writeSkillLock for commands, best-effort upsertSkillLockEntry for silent channels). * recordedLinks = the skill's previously recorded fan-out paths from the lock; lets the * fan-out replace copy-fallback artifacts and keeps unvisited paths reclaimable. + * downloadAttempts = registry fetch attempts (undefined ā†’ interactive default; silent + * background channels pass 1 to fail fast instead of stalling the host command). */ export async function installSkillWithFanout( name: string, entry: SkillIndexEntry, agents: AgentTarget[] = detectInstalledAgents(), recordedLinks: string[] = [], + downloadAttempts?: number, ): Promise { - await installSkill(name, entry); + await installSkill(name, entry, downloadAttempts); const fanout = fanOutSkillToAgents(name, agents, recordedLinks); return { lockEntry: buildSkillLockEntry(entry, fanout.links), diff --git a/packages/core/src/skills/registry.ts b/packages/core/src/skills/registry.ts index 9bbf2602b..1ae1f5bb6 100644 --- a/packages/core/src/skills/registry.ts +++ b/packages/core/src/skills/registry.ts @@ -1,5 +1,6 @@ import { BailianError } from "../errors/base.ts"; import { ExitCode } from "../errors/codes.ts"; +import { withRetry } from "../utils/retry.ts"; import type { SkillIndexEntry, SkillsIndex } from "./types.ts"; /** @@ -9,8 +10,10 @@ import type { SkillIndexEntry, SkillsIndex } from "./types.ts"; */ const DEFAULT_REGISTRY_BASE_URL = "https://bailian-wiki.oss-cn-hangzhou.aliyuncs.com/skills"; -const INDEX_TIMEOUT_MS = 10_000; +const INDEX_TIMEOUT_MS = 30_000; const ASSET_TIMEOUT_MS = 120_000; +/** Interactive channels retry transient failures; silent background channels pass 1 to fail fast */ +const DEFAULT_ATTEMPTS = 3; export function getSkillRegistryBaseUrl(): string { const override = process.env.BAILIAN_SKILL_REGISTRY_URL?.trim(); @@ -20,55 +23,64 @@ export function getSkillRegistryBaseUrl(): string { /** * Fetch the remote skill index. No local caching ā€” the diff comparison is always * "live remote index vs local skill-lock.json". - * Silent background channels (advisor sync) may pass a tighter timeout than the interactive default. + * Silent background channels (advisor sync) may pass a tighter timeout and attempts=1 + * than the interactive defaults. */ -export async function fetchSkillsIndex(timeoutMs: number = INDEX_TIMEOUT_MS): Promise { - const url = `${getSkillRegistryBaseUrl()}/index.json`; - let res: Response; - try { - res = await fetch(url, { signal: AbortSignal.timeout(timeoutMs) }); - } catch (err) { - throw new BailianError( - `Cannot access skill registry: ${url}`, - ExitCode.NETWORK, - "Check network connectivity; if using a private mirror, verify BAILIAN_SKILL_REGISTRY_URL configuration", - { cause: err }, - ); - } - if (!res.ok) { - throw new BailianError( - `Skill registry returned HTTP ${res.status}: ${url}`, - ExitCode.NETWORK, - res.status === 404 - ? "Skill index not yet published or registry URL is incorrect; confirm the publisher has generated index.json" - : "Remote error, retry later", - ); - } - let parsed: unknown; - try { - parsed = await res.json(); - } catch (err) { - throw new BailianError( - "Skill index index.json is not valid JSON", - ExitCode.GENERAL, - "Remote may be in the middle of publishing, retry later", - { cause: err }, - ); - } - const index = parsed as SkillsIndex; - if ( - typeof index !== "object" || - index === null || - typeof index.skills !== "object" || - index.skills === null - ) { - throw new BailianError( - "Skill index index.json has invalid structure", - ExitCode.GENERAL, - "Retry later or contact the publisher", - ); - } - return index; +export async function fetchSkillsIndex( + timeoutMs: number = INDEX_TIMEOUT_MS, + attempts: number = DEFAULT_ATTEMPTS, +): Promise { + return withRetry( + async () => { + const url = `${getSkillRegistryBaseUrl()}/index.json`; + let res: Response; + try { + res = await fetch(url, { signal: AbortSignal.timeout(timeoutMs) }); + } catch (err) { + throw new BailianError( + `Cannot access skill registry: ${url}`, + ExitCode.NETWORK, + "Check network connectivity; if using a private mirror, verify BAILIAN_SKILL_REGISTRY_URL configuration", + { cause: err }, + ); + } + if (!res.ok) { + throw new BailianError( + `Skill registry returned HTTP ${res.status}: ${url}`, + ExitCode.NETWORK, + res.status === 404 + ? "Skill index not yet published or registry URL is incorrect; confirm the publisher has generated index.json" + : "Remote error, retry later", + ); + } + let parsed: unknown; + try { + parsed = await res.json(); + } catch (err) { + throw new BailianError( + "Skill index index.json is not valid JSON", + ExitCode.GENERAL, + "Remote may be in the middle of publishing, retry later", + { cause: err }, + ); + } + const index = parsed as SkillsIndex; + if ( + typeof index !== "object" || + index === null || + typeof index.skills !== "object" || + index.skills === null + ) { + throw new BailianError( + "Skill index index.json has invalid structure", + ExitCode.GENERAL, + "Retry later or contact the publisher", + ); + } + return index; + }, + { attempts }, + ); } /** @@ -84,29 +96,38 @@ export function resolveAssetFileName(entry?: SkillIndexEntry): string { } /** Download the tar.br archive for a single skill (one skill = one GET) */ -export async function downloadSkillAsset(name: string, entry?: SkillIndexEntry): Promise { - const url = `${getSkillRegistryBaseUrl()}/${name}/${resolveAssetFileName(entry)}`; - let res: Response; - try { - res = await fetch(url, { signal: AbortSignal.timeout(ASSET_TIMEOUT_MS) }); - } catch (err) { - throw new BailianError( - `Failed to download skill ${name}: ${url}`, - ExitCode.NETWORK, - "Network error, retryable", - { - cause: err, - }, - ); - } - if (!res.ok) { - throw new BailianError( - `Failed to download skill ${name}: HTTP ${res.status}`, - ExitCode.NETWORK, - res.status === 404 - ? "index.json and skill object are temporarily inconsistent (publishing in progress), retry later" - : "Remote error, retry later", - ); - } - return Buffer.from(await res.arrayBuffer()); +export async function downloadSkillAsset( + name: string, + entry?: SkillIndexEntry, + attempts: number = DEFAULT_ATTEMPTS, +): Promise { + return withRetry( + async () => { + const url = `${getSkillRegistryBaseUrl()}/${name}/${resolveAssetFileName(entry)}`; + let res: Response; + try { + res = await fetch(url, { signal: AbortSignal.timeout(ASSET_TIMEOUT_MS) }); + } catch (err) { + throw new BailianError( + `Failed to download skill ${name}: ${url}`, + ExitCode.NETWORK, + "Network error, retryable", + { + cause: err, + }, + ); + } + if (!res.ok) { + throw new BailianError( + `Failed to download skill ${name}: HTTP ${res.status}`, + ExitCode.NETWORK, + res.status === 404 + ? "index.json and skill object are temporarily inconsistent (publishing in progress), retry later" + : "Remote error, retry later", + ); + } + return Buffer.from(await res.arrayBuffer()); + }, + { attempts }, + ); } diff --git a/packages/core/tests/skills-installer.test.ts b/packages/core/tests/skills-installer.test.ts index 13dfcb79a..32f828299 100644 --- a/packages/core/tests/skills-installer.test.ts +++ b/packages/core/tests/skills-installer.test.ts @@ -1,8 +1,8 @@ -import { existsSync, lstatSync, mkdtempSync, readFileSync, readdirSync, rmSync } from "fs"; -import { createHash } from "crypto"; -import { tmpdir } from "os"; -import { join } from "path"; -import { brotliCompressSync } from "zlib"; +import { existsSync, lstatSync, mkdtempSync, readFileSync, readdirSync, rmSync } from "node:fs"; +import { createHash } from "node:crypto"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { brotliCompressSync } from "node:zlib"; import tar from "tar-stream"; import { afterEach, expect, test, vi } from "vite-plus/test"; import { BailianError } from "../src/errors/base.ts"; @@ -10,6 +10,18 @@ import type { AgentTarget } from "../src/skills/agents.ts"; import { isSafeEntryName } from "../src/skills/extract.ts"; import { installSkillFromBuffer, installSkillWithFanout } from "../src/skills/installer.ts"; import { getSkillsDir } from "../src/skills/lock.ts"; +import { downloadSkillAsset, fetchSkillsIndex } from "../src/skills/registry.ts"; + +/** rmSync wrapped in a spy so tests can simulate host deletion guards (e.g. safe-delete) */ +const fsMocks = vi.hoisted(() => ({ + rmSync: vi.fn(), +})); + +vi.mock("node:fs", async (importOriginal) => { + const actual = await importOriginal(); + fsMocks.rmSync.mockImplementation(actual.rmSync); + return { ...actual, rmSync: fsMocks.rmSync }; +}); /** Run in an isolated temp config dir, restore env afterwards. */ async function inTempConfigDir(fn: () => Promise): Promise { @@ -225,15 +237,83 @@ test("fanout install: downloads, links agents, and builds lock entry with merged }); }); -test("fanout install: download failure surfaces as BailianError and leaves no canonical dir", async () => { +test("fanout install: download failure exhausts retries and leaves no canonical dir", async () => { await inTempConfigDir(async () => { - vi.stubGlobal( - "fetch", - vi.fn(async () => ({ ok: false, status: 404 })), - ); + const fetchMock = vi.fn(async () => ({ ok: false, status: 404 })); + vi.stubGlobal("fetch", fetchMock); await expect( installSkillWithFanout("demo", { contentHash: "sha256:whatever" }, []), ).rejects.toThrow(BailianError); + expect(fetchMock).toHaveBeenCalledTimes(3); expect(existsSync(join(getSkillsDir(), "demo"))).toBe(false); }); }); + +// ---- host deletion guards (safe-delete): backup cleanup must not fail a completed install ---- + +const GUARD_ERROR = + '[safe-delete][SAFE_DELETE_BULK_CONFIRM_REQUIRED] {"count":1583,"threshold":500}'; + +test("installer: guard blocking backup deletion does not fail the install", async () => { + await inTempConfigDir(async () => { + await installSkillFromBuffer("demo", await buildTarBr({ "SKILL.md": VALID_SKILL_MD })); + fsMocks.rmSync.mockImplementationOnce(() => { + throw new Error(GUARD_ERROR); + }); + const v2 = "---\nname: demo\ndescription: demo skill v2\n---\n"; + const installed = await installSkillFromBuffer("demo", await buildTarBr({ "SKILL.md": v2 })); + expect(installed.name).toBe("demo"); + expect(readFileSync(join(getSkillsDir(), "demo", "SKILL.md"), "utf-8")).toBe(v2); + // The blocked backup stays on disk but is inert (status scans ignore .old-*) + const leftovers = readdirSync(getSkillsDir()).filter((entry) => entry.startsWith("demo.old-")); + expect(leftovers).toHaveLength(1); + }); +}); + +test("installer: temp cleanup failure does not mask the original error", async () => { + await inTempConfigDir(async () => { + fsMocks.rmSync.mockImplementationOnce(() => { + throw new Error(GUARD_ERROR); + }); + const buf = await buildTarBr({ "SKILL.md": VALID_SKILL_MD, "../evil.txt": "pwned\n" }); + await expect(installSkillFromBuffer("demo", buf)).rejects.toThrow(/unsafe tar entry/); + }); +}); + +// ---- registry retry policy ---- + +test("registry: index fetch retries transient failures and succeeds", async () => { + const indexPayload = { skills: { demo: { contentHash: "sha256:abc" } } }; + const fetchMock = vi.fn(async () => { + if (fetchMock.mock.calls.length < 3) throw new Error("network down"); + return { ok: true, status: 200, json: async () => indexPayload }; + }); + vi.stubGlobal("fetch", fetchMock); + const index = await fetchSkillsIndex(1000); + expect(index.skills.demo.contentHash).toBe("sha256:abc"); + expect(fetchMock).toHaveBeenCalledTimes(3); +}); + +test("registry: attempts=1 keeps the silent channel fail-fast", async () => { + const fetchMock = vi.fn(async () => { + throw new Error("network down"); + }); + vi.stubGlobal("fetch", fetchMock); + await expect(fetchSkillsIndex(1000, 1)).rejects.toThrow(BailianError); + expect(fetchMock).toHaveBeenCalledTimes(1); +}); + +test("registry: asset download retries transient HTTP errors", async () => { + const fetchMock = vi.fn(async () => { + if (fetchMock.mock.calls.length === 1) return { ok: false, status: 503 }; + return { + ok: true, + status: 200, + arrayBuffer: async () => new Uint8Array([1, 2, 3]).buffer, + }; + }); + vi.stubGlobal("fetch", fetchMock); + const buffer = await downloadSkillAsset("demo"); + expect([...buffer]).toEqual([1, 2, 3]); + expect(fetchMock).toHaveBeenCalledTimes(2); +});