-
Notifications
You must be signed in to change notification settings - Fork 3.1k
perf(cli): avoid hashing invalid inline image cache hits #10626
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
390f4e9
141e2fb
2fb460e
9803b78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,13 +4,15 @@ | |
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import crypto from 'node:crypto'; | ||
| import fs from 'node:fs/promises'; | ||
| import os from 'node:os'; | ||
| import path from 'node:path'; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import { | ||
| containsCmdShellMetacharacters, | ||
| getTerminalImageRenderSupport, | ||
| INLINE_DECODE_NEGATIVE_CACHE_BYTE_LIMIT, | ||
| INLINE_DECODE_NEGATIVE_CACHE_LIMIT, | ||
| MAX_INLINE_IMAGE_PIXELS, | ||
| markKittyImageWritten, | ||
|
|
@@ -133,52 +135,120 @@ describe('terminalImageRenderer', () => { | |
| } | ||
| }); | ||
|
|
||
| it('caches repeated invalid inline payloads and evicts old entries', () => { | ||
| it('caches invalid inline payloads with bounded LRU eviction', () => { | ||
| const sharedPrefix = 'A'.repeat(64); | ||
| const invalidPayloads = Array.from( | ||
| { length: INLINE_DECODE_NEGATIVE_CACHE_LIMIT + 1 }, | ||
| (_, index) => Buffer.from(`not a png ${index}`).toString('base64'), | ||
| (_, index) => Buffer.from(`${sharedPrefix}${index}`).toString('base64'), | ||
| ); | ||
| const bufferFrom = vi.spyOn(Buffer, 'from'); | ||
|
|
||
| try { | ||
| for (const data of invalidPayloads) { | ||
| prepareInlineTerminalImage({ | ||
| data, | ||
| mimeType: 'image/png', | ||
| contentWidth: 24, | ||
| env: { TERM: 'xterm-kitty' }, | ||
| stdoutIsTTY: true, | ||
| }); | ||
| } | ||
|
|
||
| const createHash = vi.spyOn(crypto, 'createHash'); | ||
| const prepare = (data: string) => | ||
| prepareInlineTerminalImage({ | ||
| data: invalidPayloads[0], | ||
| data, | ||
| mimeType: 'image/png', | ||
| contentWidth: 24, | ||
| env: { TERM: 'xterm-kitty' }, | ||
| stdoutIsTTY: true, | ||
| }); | ||
|
|
||
| try { | ||
| for (const data of invalidPayloads.slice( | ||
| 0, | ||
| INLINE_DECODE_NEGATIVE_CACHE_LIMIT, | ||
| )) { | ||
| prepare(data); | ||
| } | ||
| expect(bufferFrom).toHaveBeenCalledTimes( | ||
| INLINE_DECODE_NEGATIVE_CACHE_LIMIT, | ||
| ); | ||
|
|
||
| prepare('A'.repeat(MAX_INLINE_IMAGE_ENCODED_LENGTH + 1)); | ||
| expect(bufferFrom).toHaveBeenCalledTimes( | ||
| INLINE_DECODE_NEGATIVE_CACHE_LIMIT, | ||
| ); | ||
|
|
||
| prepare(invalidPayloads[0]); | ||
| expect(bufferFrom).toHaveBeenCalledTimes( | ||
| INLINE_DECODE_NEGATIVE_CACHE_LIMIT, | ||
| ); | ||
|
|
||
| prepare(invalidPayloads[INLINE_DECODE_NEGATIVE_CACHE_LIMIT]); | ||
| expect(bufferFrom).toHaveBeenCalledTimes( | ||
| INLINE_DECODE_NEGATIVE_CACHE_LIMIT + 1, | ||
| ); | ||
|
|
||
| prepare(invalidPayloads[0]); | ||
| expect(bufferFrom).toHaveBeenCalledTimes( | ||
| INLINE_DECODE_NEGATIVE_CACHE_LIMIT + 1, | ||
| ); | ||
|
|
||
| prepare(invalidPayloads[1]); | ||
| expect(bufferFrom).toHaveBeenCalledTimes( | ||
| INLINE_DECODE_NEGATIVE_CACHE_LIMIT + 2, | ||
| ); | ||
| expect(createHash).not.toHaveBeenCalled(); | ||
| } finally { | ||
| bufferFrom.mockRestore(); | ||
| createHash.mockRestore(); | ||
| } | ||
| }); | ||
|
|
||
| it('rejects inline payloads above the shared image limit before decoding', () => { | ||
| const oversizedBase64 = 'A'.repeat(MAX_INLINE_IMAGE_ENCODED_LENGTH + 1); | ||
|
|
||
| expect( | ||
| it('bounds invalid inline payload cache entries by total bytes', () => { | ||
| const invalidPayloads = Array.from({ length: 3 }, (_, index) => | ||
| Buffer.from(`invalid byte budget payload ${index}`).toString('base64'), | ||
| ); | ||
| const bufferFrom = vi.spyOn(Buffer, 'from'); | ||
| const byteLength = vi | ||
| .spyOn(Buffer, 'byteLength') | ||
| .mockReturnValue(INLINE_DECODE_NEGATIVE_CACHE_BYTE_LIMIT / 2); | ||
| const prepare = (data: string) => | ||
| prepareInlineTerminalImage({ | ||
| data: oversizedBase64, | ||
| data, | ||
| mimeType: 'image/png', | ||
| contentWidth: 24, | ||
| env: { TERM: 'xterm-kitty' }, | ||
| stdoutIsTTY: true, | ||
| }), | ||
| ).toEqual({ fallbackText: '[image: png]', result: null }); | ||
| }); | ||
|
|
||
| try { | ||
| prepare(invalidPayloads[0]); | ||
| prepare(invalidPayloads[1]); | ||
| prepare(invalidPayloads[0]); | ||
| prepare(invalidPayloads[2]); | ||
| prepare(invalidPayloads[0]); | ||
| expect(bufferFrom).toHaveBeenCalledTimes(3); | ||
|
|
||
| prepare(invalidPayloads[1]); | ||
| expect(bufferFrom).toHaveBeenCalledTimes(4); | ||
| expect(byteLength).toHaveBeenCalledTimes(4); | ||
| } finally { | ||
| bufferFrom.mockRestore(); | ||
| byteLength.mockRestore(); | ||
| } | ||
| }); | ||
|
|
||
| it('rejects inline payloads above the shared image limit before decoding', () => { | ||
| const oversizedBase64 = 'A'.repeat(MAX_INLINE_IMAGE_ENCODED_LENGTH + 1); | ||
| const bufferFrom = vi.spyOn(Buffer, 'from'); | ||
| const createHash = vi.spyOn(crypto, 'createHash'); | ||
|
|
||
| try { | ||
| expect( | ||
| prepareInlineTerminalImage({ | ||
| data: oversizedBase64, | ||
| mimeType: 'image/png', | ||
| contentWidth: 24, | ||
| env: { TERM: 'xterm-kitty' }, | ||
| stdoutIsTTY: true, | ||
| }), | ||
| ).toEqual({ fallbackText: '[image: png]', result: null }); | ||
| expect(bufferFrom).not.toHaveBeenCalled(); | ||
| expect(createHash).not.toHaveBeenCalled(); | ||
| } finally { | ||
|
Comment on lines
+246
to
+248
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R1-1: Still stands — re-reported from round 1 (the branch code is unchanged since; only a merge from Witness: Suggested fix (unchanged from round 1): spy on it('rejects inline payloads above the shared image limit before decoding', () => {
const oversizedBase64 = 'A'.repeat(MAX_INLINE_IMAGE_ENCODED_LENGTH + 1);
const bufferFrom = vi.spyOn(Buffer, 'from');
const createHash = vi.spyOn(crypto, 'createHash');
try {
expect(
prepareInlineTerminalImage({
data: oversizedBase64,
mimeType: 'image/png',
contentWidth: 24,
env: { TERM: 'xterm-kitty' },
stdoutIsTTY: true,
}),
).toEqual({ fallbackText: '[image: png]', result: null });
expect(bufferFrom).not.toHaveBeenCalled();
expect(createHash).not.toHaveBeenCalled();
} finally {
bufferFrom.mockRestore();
createHash.mockRestore();
}
});Keep the assertion scoped to this oversized/invalid flow — 中文说明(仍然成立 — 自第 1 轮起重新报告;此后分支代码未变,仅合并了 探针证据:MUTANT(长度检查移回缓存查询之后):Tests 31 passed (31),callsAfterResubmit=9 junk0-redecoded=true;INTACT(本 PR 代码):callsAfterResubmit=8 junk0-redecoded=false。 建议的修复(与第 1 轮相同):在此处对 约束:该断言须保持在超限/无效流程内—— — qwen3.8-max via Qwen Code /review (v0.22.3)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in The round-2 cache-pollution witness is covered too: after filling all 64 resident entries, the test submits an oversized payload and confirms that the original oldest entry is still resident. Moving the length guard back into the decoder makes that assertion fail through an extra decode. Payloads in the count/LRU test share a 64-character prefix, which also makes the former N5 prefix-key mutation fail. The focused file passes 32/32 tests, and an independent verifier killed all five relevant mutations: missing hit refresh, guard-down/cache pollution, missing byte bound, prefix key, and restored SHA key. 中文说明已在 第 2 轮指出的缓存污染也已有承重见证:测试填满全部 64 个驻留项后提交超限 payload,并确认原本最旧的条目仍然驻留。把长度 guard 移回 decoder 会导致额外一次解码,从而让该断言失败。count/LRU 测试中的 payload 共享 64 字符前缀,因此此前的 N5 前缀 key 变异也会失败。 目标测试文件 32/32 通过;独立 verifier 杀死了五类相关变异:删除命中刷新、guard 下移并污染缓存、删除字节上限、前缀 key,以及恢复 SHA key。 |
||
| bufferFrom.mockRestore(); | ||
| createHash.mockRestore(); | ||
| } | ||
| }); | ||
|
|
||
| it('rejects inline PNG dimensions above the shared image limit', () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -52,7 +52,11 @@ const inlineDecodeCache = new Map< | |||||||||||||||
| { png: Buffer; size: { width: number; height: number } } | ||||||||||||||||
| >(); | ||||||||||||||||
| export const INLINE_DECODE_NEGATIVE_CACHE_LIMIT = 64; | ||||||||||||||||
| const invalidInlineImageCache = new Set<string>(); | ||||||||||||||||
| // Preserve the eight-entry cache's worst-case raw-key budget for large payloads. | ||||||||||||||||
| export const INLINE_DECODE_NEGATIVE_CACHE_BYTE_LIMIT = | ||||||||||||||||
| 8 * MAX_INLINE_IMAGE_ENCODED_LENGTH; | ||||||||||||||||
|
Comment on lines
+55
to
+57
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R3-1: The rationale comment for this byte budget references an "eight-entry cache" that does not exist in the merged code. The Witness:
Suggested change
The budget value itself was measured and accepted in this PR's discussion (8 × 中文说明此字节上限的理由注释引用了一个在合并后代码中并不存在的"八条目缓存"。 证据(Witness)与建议修复(suggestion 代码块)见上方英文部分。预算值本身已经过测量并在本 PR 讨论中被接受(8 × — qwen3.8-max via Qwen Code /review (v0.23.0) |
||||||||||||||||
| const invalidInlineImageCache = new Map<string, number>(); | ||||||||||||||||
| let invalidInlineImageCacheBytes = 0; | ||||||||||||||||
|
|
||||||||||||||||
| // A Kitty terminal keeps a transmitted image and redraws it from the placeholder | ||||||||||||||||
| // cells alone. The live-row -> Static-row move and every resize remount | ||||||||||||||||
|
|
@@ -346,10 +350,6 @@ function getImageFormat(mimeType: string): string | null { | |||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| function decodeInlineImage(data: string): Buffer | null { | ||||||||||||||||
| if (data.length === 0 || data.length > MAX_INLINE_IMAGE_ENCODED_LENGTH) { | ||||||||||||||||
| return null; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| const normalized = data.replace(/\s/g, ''); | ||||||||||||||||
| if ( | ||||||||||||||||
| normalized.length === 0 || | ||||||||||||||||
|
|
@@ -374,28 +374,39 @@ function decodeInlineImage(data: string): Buffer | null { | |||||||||||||||
| function getDecodedInlinePng( | ||||||||||||||||
| data: string, | ||||||||||||||||
| ): { png: Buffer; size: { width: number; height: number } } | null { | ||||||||||||||||
| if (data.length === 0 || data.length > MAX_INLINE_IMAGE_ENCODED_LENGTH) { | ||||||||||||||||
| return null; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| const cached = inlineDecodeCache.get(data); | ||||||||||||||||
| if (cached) { | ||||||||||||||||
| inlineDecodeCache.delete(data); | ||||||||||||||||
| inlineDecodeCache.set(data, cached); | ||||||||||||||||
| return cached; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| const negativeKey = crypto.createHash('sha256').update(data).digest('hex'); | ||||||||||||||||
| if (invalidInlineImageCache.has(negativeKey)) { | ||||||||||||||||
| invalidInlineImageCache.delete(negativeKey); | ||||||||||||||||
| invalidInlineImageCache.add(negativeKey); | ||||||||||||||||
| const invalidBytes = invalidInlineImageCache.get(data); | ||||||||||||||||
| if (invalidBytes !== undefined) { | ||||||||||||||||
| invalidInlineImageCache.delete(data); | ||||||||||||||||
| invalidInlineImageCache.set(data, invalidBytes); | ||||||||||||||||
| return null; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| const png = decodeInlineImage(data); | ||||||||||||||||
| if (!png) { | ||||||||||||||||
| cacheInvalidInlineImage(negativeKey); | ||||||||||||||||
| return null; | ||||||||||||||||
| } | ||||||||||||||||
| const size = readValidatedInlinePngSize(png); | ||||||||||||||||
| if (!size) { | ||||||||||||||||
| cacheInvalidInlineImage(negativeKey); | ||||||||||||||||
| const size = png ? readValidatedInlinePngSize(png) : null; | ||||||||||||||||
| if (!png || !size) { | ||||||||||||||||
| const dataBytes = Buffer.byteLength(data); | ||||||||||||||||
| invalidInlineImageCache.set(data, dataBytes); | ||||||||||||||||
| invalidInlineImageCacheBytes += dataBytes; | ||||||||||||||||
| while ( | ||||||||||||||||
| invalidInlineImageCache.size > INLINE_DECODE_NEGATIVE_CACHE_LIMIT || | ||||||||||||||||
| invalidInlineImageCacheBytes > INLINE_DECODE_NEGATIVE_CACHE_BYTE_LIMIT | ||||||||||||||||
| ) { | ||||||||||||||||
| const oldest = invalidInlineImageCache.entries().next().value; | ||||||||||||||||
| if (oldest === undefined) break; | ||||||||||||||||
| invalidInlineImageCache.delete(oldest[0]); | ||||||||||||||||
| invalidInlineImageCacheBytes -= oldest[1]; | ||||||||||||||||
| } | ||||||||||||||||
| return null; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -409,15 +420,6 @@ function getDecodedInlinePng( | |||||||||||||||
| return decoded; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| function cacheInvalidInlineImage(key: string): void { | ||||||||||||||||
| invalidInlineImageCache.add(key); | ||||||||||||||||
| while (invalidInlineImageCache.size > INLINE_DECODE_NEGATIVE_CACHE_LIMIT) { | ||||||||||||||||
| const oldest = invalidInlineImageCache.values().next().value; | ||||||||||||||||
| if (oldest === undefined) break; | ||||||||||||||||
| invalidInlineImageCache.delete(oldest); | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| function readValidatedInlinePngSize( | ||||||||||||||||
| png: Buffer, | ||||||||||||||||
| ): { width: number; height: number } | null { | ||||||||||||||||
|
|
||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] R1-1: The
expect(bufferFrom).not.toHaveBeenCalled()assertion added here passes identically against the pre-change code — the old size guard insidedecodeInlineImagealready ran before anyBuffer.fromcall — so it pins nothing about the actual behavioral change of this hunk: oversized payloads now return before the negative-cache lookup and are never hashed or negatively cached. Pre-change,getDecodedInlinePngcomputedcrypto.createHash('sha256')over every payload — oversized ones included — before the size check ran, and negatively cached the result. If a future edit moves the length check back below the cache lookups, every test in this file stays green while every oversized call again hashes an up-to-~11 MB string and occupies one of the 8 negative-cache slots, evicting useful entries — silently resurrecting the exact cost this PR removes. Spy oncreateHashhere (as the LRU test above already does) and assert it is not called.Witness:
The fix spans three spots (spy setup, assertion, restore):
Keep the assertion scoped to this oversized/invalid flow:
crypto.createHashis still legitimately invoked on the successful-render path —createInlineRenderCacheKey(terminal-image-renderer.ts:334) andcreateImageId(:541) — which this test never reaches. Once the fix is in, removing it must turnrejects inline payloads above the shared image limit before decodingred — verify by moving the length check back intodecodeInlineImageand confirming the new assertion fails while the existingbufferFromone still passes.中文说明
此处新增的
expect(bufferFrom).not.toHaveBeenCalled()断言在改动前的代码上同样通过——旧的大小检查位于decodeInlineImage内部、任何Buffer.from调用之前——因此它并没有钉住本处真正的行为变化:超限 payload 现在会在查询负缓存之前直接返回,既不会被哈希,也不会进入负缓存。改动前,getDecodedInlinePng会在大小检查运行前对每个 payload(包括超限的)计算crypto.createHash('sha256'),并将哈希结果存入负缓存。如果未来的修改把长度检查移回缓存查询之后,本文件所有测试仍会全绿,而每次超限调用又会重新哈希一个最长约 11 MB 的字符串并占用 8 个负缓存槽位之一、挤掉有用条目——悄悄复活本 PR 要消除的开销。请在此处对createHash打桩(上方 LRU 测试已有同样写法)并断言它未被调用。探针证据(临时树中四组对照):改动前源码 + 本 PR 测试(含 bufferFrom 断言):"Tests 1 passed"——该 mutation 在现有断言下存活;改动前源码 + 建议新增的 expect(createHash).not.toHaveBeenCalled():"1 failed",正失败在该断言上;本 PR 源码 + 建议断言:"1 passed"。建议的断言在本次改动前后发生红/绿翻转,而现有断言不翻转。
修复涉及三处(打桩、断言、恢复),完整代码见上方英文部分的代码块。
约束:该断言须保持在超限/无效流程内——
crypto.createHash在渲染成功路径上仍被正常使用(createInlineRenderCacheKey,terminal-image-renderer.ts:334;createImageId,:541),本测试不会走到这些路径。修复落地后,若将其移除,rejects inline payloads above the shared image limit before decoding必须变红——可将长度检查移回decodeInlineImage验证:新断言失败,而现有bufferFrom断言仍通过。— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in
2fb460ef0957e2c09e6f631a7fb173af924055fe. The oversized-path test now spies oncrypto.createHashand asserts zero calls, as suggested.I also strengthened the resident-cache witness because the standalone SHA assertion alone does not detect every cache-pollution form once the cache uses raw keys: the LRU test fills all 64 resident entries, submits an oversized payload, and then revisits the original oldest entry. Moving the guard back into the decoder now evicts that resident and makes the test fail with an extra decode. The same test uses payloads sharing a 64-character prefix, so a truncated-key implementation fails as well.
The focused file passes 32/32 tests. Independent mutation checks killed the guard/cache-pollution regression, restored SHA key, 64-character prefix key, missing LRU refresh, and missing byte-limit branch.
中文说明
已在
2fb460ef0957e2c09e6f631a7fb173af924055fe中处理。超限路径测试现在按建议对crypto.createHash打桩并断言调用次数为零。同时加固了驻留缓存见证,因为负缓存改用 raw key 后,单独的零 SHA 断言并不能识别所有缓存污染形式:LRU 测试先填满 64 个驻留项,再提交一个超限 payload,最后复访原本最旧的条目。把 guard 移回 decoder 会挤掉该驻留项并产生额外一次解码,因此测试会失败。该测试的 payload 还共享 64 字符前缀,所以截断 key 的实现同样会失败。
目标测试文件 32/32 通过。独立变异验证已经杀死 guard/缓存污染、恢复 SHA key、64 字符前缀 key、删除 LRU refresh 和删除 byte-limit 分支五类回归。