fix(cli): make app-info cache resilient to corrupt entries + route errors through structured envelope (WTA-257) (#37852)

This commit is contained in:
Xiyuan Chen 2026-06-24 00:39:26 -07:00 committed by GitHub
parent 2112115962
commit 32dc9ff2d9
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 144 additions and 28 deletions

View File

@ -1,13 +1,14 @@
import type { DifyMock } from '@test/fixtures/dify-mock/server'
import { mkdtemp, rm } from 'node:fs/promises'
import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { startMock } from '@test/fixtures/dify-mock/server'
import { testHttpClient } from '@test/fixtures/http-client'
import yaml from 'js-yaml'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { loadAppInfoCache } from '@/cache/app-info'
import { ENV_CACHE_DIR } from '@/store/dir'
import { CACHE_APP_INFO, getCache } from '@/store/manager'
import { CACHE_APP_INFO, cachePath, getCache } from '@/store/manager'
import { FieldInfo, FieldParameters } from '@/types/app-meta'
import { AppMetaClient } from './app-meta.js'
import { AppsClient } from './apps.js'
@ -97,4 +98,40 @@ describe('AppMetaClient', () => {
await client.get('app-1', [FieldInfo])
expect(spy).toHaveBeenCalledTimes(2)
})
it('corrupt cache entry refetches; valid sibling stays cached; no throw', async () => {
const path = cachePath(dir, CACHE_APP_INFO)
const apps = new AppsClient(testHttpClient(mock.url, 'dfoa_test'))
// Seed a real, production-serialized entry by fetching app-1 once (this
// calls cache.set → serialize, so we never hand-write the on-disk shape).
const seed = await loadAppInfoCache({ store: getCache(CACHE_APP_INFO) })
await new AppMetaClient({ apps, host: mock.url, cache: seed }).get('app-1', [FieldInfo])
// Reuse that serialized entry as a valid sibling; corrupt the app-1 slot.
const file = yaml.load(await readFile(path, 'utf8')) as { entries: Record<string, unknown> }
const validEntry = file.entries[`${mock.url}::app-1`]
await writeFile(
path,
yaml.dump({ entries: {
[`${mock.url}::app-1`]: 'corrupted-string',
[`${mock.url}::sibling`]: validEntry,
} }),
'utf8',
)
// Reload: app-1 dropped, sibling kept.
const cache = await loadAppInfoCache({ store: getCache(CACHE_APP_INFO) })
const spy = vi.spyOn(apps, 'describe')
const client = new AppMetaClient({ apps, host: mock.url, cache })
// app-1 corrupt → dropped → miss → refetched from the mock
const a = await client.get('app-1', [FieldInfo])
expect(a.info?.id).toBe('app-1')
expect(spy).toHaveBeenCalledTimes(1)
// sibling is the real serialized entry → served from cache, no network
await client.get('sibling', [FieldInfo])
expect(spy).toHaveBeenCalledTimes(1)
})
})

View File

@ -1,5 +1,5 @@
import type { AppMeta } from '@/types/app-meta'
import { mkdtemp, readFile, rm } from 'node:fs/promises'
import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import yaml from 'js-yaml'
@ -14,10 +14,10 @@ function appInfoPath(dir: string): string {
return cachePath(dir, CACHE_APP_INFO)
}
function metaInfoOnly(): AppMeta {
function metaInfoOnly(id = 'app-1'): AppMeta {
return {
info: {
id: 'app-1',
id,
name: 'Greeter',
description: '',
mode: 'chat',
@ -101,12 +101,32 @@ describe('app-info disk cache', () => {
})
it('corrupt cache file is treated as empty', async () => {
const { writeFile } = await import('node:fs/promises')
await writeFile(appInfoPath(dir), ': : not valid yaml', 'utf8')
const c = await loadAppInfoCache({ store: getCache(CACHE_APP_INFO) })
expect(c.get('h', 'app-1')).toBeUndefined()
})
it('drops a corrupt single entry but keeps valid siblings', async () => {
// Seed a real serialized entry via set() — no hand-authored on-disk shape.
const seed = await loadAppInfoCache({ store: getCache(CACHE_APP_INFO) })
await seed.set('h', 'app-2', metaInfoOnly('app-2'))
// Inject a corrupt sibling alongside the real one.
const file = yaml.load(await readFile(appInfoPath(dir), 'utf8')) as { entries: Record<string, unknown> }
file.entries['h::app-1'] = 'corrupted-string-not-object'
await writeFile(appInfoPath(dir), yaml.dump(file), 'utf8')
const c = await loadAppInfoCache({ store: getCache(CACHE_APP_INFO) })
expect(c.get('h', 'app-1')).toBeUndefined()
expect(c.get('h', 'app-2')?.meta.info?.id).toBe('app-2')
})
it('treats a non-object entries map as empty', async () => {
await writeFile(appInfoPath(dir), yaml.dump({ entries: 'not-an-object' }), 'utf8')
const c = await loadAppInfoCache({ store: getCache(CACHE_APP_INFO) })
expect(c.get('h', 'app-1')).toBeUndefined()
})
it('updates same key in place (no growth)', async () => {
const c = await loadAppInfoCache({ store: getCache(CACHE_APP_INFO) })
await c.set('h', 'app-1', metaInfoOnly())

View File

@ -74,8 +74,18 @@ async function readEntries(store: Store): Promise<Map<string, AppMetaCacheRecord
catch {
return out
}
for (const [k, e] of Object.entries(raw))
out.set(k, deserialize(e))
// A scalar/array survives Object.entries as garbage rather than throwing.
if (raw === null || typeof raw !== 'object' || Array.isArray(raw))
return out
for (const [k, e] of Object.entries(raw)) {
try {
out.set(k, deserialize(e))
}
catch {
// Drop unreadable entry → becomes a cache miss → consumer refetches.
}
}
return out
}

View File

@ -212,18 +212,30 @@ describe('run() catch routing', () => {
expect(result.exit).toBe(ExitCode.Generic)
})
it('falls through to generic Error branch and exits 1', async () => {
it('routes non-BaseError to JSON envelope with -o json (exit 1)', async () => {
class Throwing extends Command {
async run(_argv: string[]) {
throw new Error('boom')
}
}
const result = await captureRun(makeTree(Throwing), ['cmd', '-o', 'json'])
expect(result.stderr).toBe(`${JSON.stringify({ error: { code: 'unknown', message: 'boom' } })}\n`)
expect(result.exit).toBe(ExitCode.Generic)
expect(result.stdout).toBe('')
})
it('wraps a generic Error into the human unknown form and exits 1', async () => {
class Throwing extends Command {
async run(_argv: string[]) {
throw new Error('oops')
}
}
const result = await captureRun(makeTree(Throwing), ['cmd'])
expect(result.stderr).toBe('oops\n')
expect(result.exit).toBe(1)
expect(result.stderr).toBe('unknown: oops\n')
expect(result.exit).toBe(ExitCode.Generic)
})
it('handles non-Error throw via String() coercion', async () => {
it('wraps a non-Error throw via String() coercion into unknown form', async () => {
class Throwing extends Command {
async run(_argv: string[]) {
// eslint-disable-next-line no-throw-literal
@ -231,8 +243,52 @@ describe('run() catch routing', () => {
}
}
const result = await captureRun(makeTree(Throwing), ['cmd'])
expect(result.stderr).toBe('plain string\n')
expect(result.exit).toBe(1)
expect(result.stderr).toBe('unknown: plain string\n')
expect(result.exit).toBe(ExitCode.Generic)
})
it('exits 0 on EPIPE without writing an error envelope', async () => {
class Throwing extends Command {
async run(_argv: string[]) {
throw Object.assign(new Error('broken pipe'), { code: 'EPIPE' })
}
}
// process.exit is typed `never`; stub it to halt (throw) like the real call,
// so the EPIPE early-exit doesn't fall through to the envelope path.
let exitCode: number | undefined
let stderr = ''
const origExit = process.exit.bind(process)
const origStderr = process.stderr.write.bind(process.stderr)
process.exit = ((code?: number) => {
exitCode = code
throw new Error('__exit__')
}) as typeof process.exit
process.stderr.write = ((chunk: string | Uint8Array) => {
stderr += typeof chunk === 'string' ? chunk : new TextDecoder().decode(chunk)
return true
}) as typeof process.stderr.write
try {
await run(makeTree(Throwing), ['cmd', '-o', 'json'])
}
catch (e) {
expect((e as Error).message).toBe('__exit__')
}
finally {
process.exit = origExit
process.stderr.write = origStderr
}
expect(exitCode).toBe(0)
expect(stderr).toBe('')
})
it('preserves RateLimited semantic exit code through the collapsed catch', async () => {
class Throwing extends Command {
async run(_argv: string[]) {
throw newError(ErrorCode.RateLimited, 'slow down')
}
}
const result = await captureRun(makeTree(Throwing), ['cmd'])
expect(result.exit).toBe(ExitCode.RateLimited)
})
it('does not call process.exit when command runs successfully', async () => {

View File

@ -1,5 +1,5 @@
import type { CommandTree } from './registry'
import { BaseError } from '@/errors/base'
import { BaseError, unknownError } from '@/errors/base'
import { formatErrorForCli } from '@/errors/format'
import { findTopic } from '@/help/topics'
import { formatCommandList, formatHelp, formatTopic, formatTopLevelHelp } from './help'
@ -106,19 +106,12 @@ export async function run(tree: CommandTree, argv: string[]): Promise<void> {
catch (err) {
if ((err as NodeJS.ErrnoException).code === 'EPIPE')
process.exit(0)
if (err instanceof BaseError) {
const format = sniffOutputFormat(argv)
process.stderr.write(`${formatErrorForCli(err, { format, isErrTTY: process.stderr.isTTY })}\n`)
process.exit(err.exit())
return
}
if (err instanceof Error) {
process.stderr.write(`${err.message}\n`)
process.exit(1)
return
}
process.stderr.write(`${String(err)}\n`)
process.exit(1)
const e = err instanceof BaseError
? err
: unknownError(err instanceof Error ? err.message : String(err), err)
const format = sniffOutputFormat(argv)
process.stderr.write(`${formatErrorForCli(e, { format, isErrTTY: process.stderr.isTTY })}\n`)
process.exit(e.exit())
}
}