From d2218f54da69849639a8b3edcea6a5fd4ece7d8c Mon Sep 17 00:00:00 2001 From: Zachary Lowden Date: Mon, 14 Sep 2026 21:02:33 -0500 Subject: [PATCH] fix(images): stop /api/v1/images coercing an all-digit username to a number (#4839) Server half of civitai/cli#513 / #4768. Pairs with the submodule fix civitai/event-engine-common#13 (5da5cc6467), which this pins. The coercion was never in Meilisearch, which is what #4768's body still says. The index document's user.username never reaches the response: getImagesFromFeedSearch -> ImagesFeed.populatedQuery builds `user` from the Postgres-backed userData Redis cache and spreads it AFTER the doc, overriding it. A Redis hash stores only strings, so createCache serialised each field on write and guessed the type back on read -- isNaN(Number(v)) ? v : Number(v). Number('0222') is 222. Discriminating observation, cache-busted with cf-cache-status MISS on every row: ?username=0222&limit=11 returned "0222" (Redis miss, raw Postgres row) and limit=12..16 returned 222 (Redis hit, decoded). A Meilisearch document cannot change between two requests seconds apart. Ships both halves: the field-type declarations in the in-repo fork of the cache (apps/event-engine/src/common/caches/, which writes the SAME Redis keys), the submodule pin, and a belt-labelled String() cast at the emit site. 447 of the lines are tests. Consumer audit posted on #13: the only LIVE defect is the username one. The 'false'-as-truthy-string and array-as-raw-text cases are real in the decoder but latent -- nothing reads modelData.nsfw from the cache (all three .nsfw reads come from a direct pg.query) and nothing calls Array.isArray on the cached arrays. NOT verified: nothing was exercised against a real Redis or a deploy, and the consumer audit covers static field reads only -- services/cache.ts uses a namespace import, so a dynamic access would not have appeared. /api/v1/blocks/images shares runImageSearch but returns 401 Block token required, so it is covered by code identity, not measurement. --- apps/event-engine/src/common/caches/base.ts | 49 ++- .../src/common/caches/imageData.cache.ts | 22 ++ .../src/common/caches/modelData.cache.ts | 7 + .../src/common/caches/userData.cache.ts | 14 + .../src/common/caches/value-codec.ts | 158 +++++++++ event-engine-common | 2 +- .../__tests__/feed-cache-value-codec.test.ts | 317 ++++++++++++++++++ .../image-search-username-type.test.ts | 130 +++++++ src/server/services/image-search.service.ts | 19 +- 9 files changed, 689 insertions(+), 29 deletions(-) create mode 100644 apps/event-engine/src/common/caches/value-codec.ts create mode 100644 src/server/services/__tests__/feed-cache-value-codec.test.ts create mode 100644 src/server/services/__tests__/image-search-username-type.test.ts diff --git a/apps/event-engine/src/common/caches/base.ts b/apps/event-engine/src/common/caches/base.ts index d1094ac3dc..c68443f036 100644 --- a/apps/event-engine/src/common/caches/base.ts +++ b/apps/event-engine/src/common/caches/base.ts @@ -1,5 +1,6 @@ import { IRedisClient } from '../types/package-stubs'; import { chunk, sleep } from '../utils/basic'; +import { decodeCacheFields, type CacheFieldTypes } from './value-codec'; const CACHE_TTL = 24 * 60 * 60; // 24 hours const MISS_CACHE_TTL = 5 * 60; // 5 minutes @@ -17,6 +18,17 @@ export type CacheContext = { export type CacheConfig = { redisKey: string; // Prefix for cached items (e.g., 'user:data') idKey: keyof T; // Property used as key in result (e.g., 'userId') + /** + * How each stored field is read back out of the Redis hash. + * + * REQUIRED, and required to be COMPLETE — the type is a non-`Partial` record + * over `keyof T`, so adding a column to `fetch`'s `SELECT` does not compile + * until it is declared. Without a declaration a field falls back to guessing + * its type from the text, which is how an all-digit username was read back as + * a number and a leading-zero one was read back as a DIFFERENT name + * (civitai#4768). See `caches/value-codec.ts`. + */ + fieldTypes: CacheFieldTypes; fetch: (ctx: CacheContext, ids: number[]) => Promise; // Fetch function returns array ttl?: number; // Default cache TTL in seconds debounceTime?: number; // Time for writes to propagate to read replicas (default 10s) @@ -86,26 +98,11 @@ export function createCache(config: CacheConfig) { continue; } - // Parse the cached data + // Parse the cached data. `value-codec` is the single place that knows + // how a field is to be read back; inferring the type from the text here + // is what destroyed all-digit usernames (civitai#4768). const cachedAt = cacheResult.cachedAt ? new Date(cacheResult.cachedAt) : new Date(0); - const item: any = {}; - - for (const [key, value] of Object.entries(cacheResult)) { - if (key === 'cachedAt') continue; - - // Try to parse as JSON first (for arrays/objects) - if (value.startsWith('[') || value.startsWith('{')) { - try { - item[key] = JSON.parse(value); - continue; - } catch { - // Not valid JSON, fall through to string/number handling - } - } - - // Try to parse as number if it looks like one - item[key] = isNaN(Number(value)) ? value : Number(value); - } + const item: any = decodeCacheFields(cacheResult, config.fieldTypes); // Check if stale and needs revalidation if (staleWhileRevalidate && cachedAt < ttlExpiry) { @@ -253,14 +250,12 @@ export function createCache(config: CacheConfig) { // Skip not found entries if (cacheResult.notFound === '1') continue; - // Parse the cached data - const item: any = {}; - for (const [key, value] of Object.entries(cacheResult)) { - if (key === 'cachedAt') continue; - item[key] = isNaN(Number(value)) ? value : Number(value); - } - - results[id] = item as T; + // Parse the cached data through the SAME codec as the uncontended + // read above. This branch used to carry its own copy of the decoder, + // and that copy had no array/object handling at all — so a cached + // `tags`/`cosmetics` array came back as the raw text '[3,9]' and + // every `Array.isArray` check downstream silently saw `false`. + results[id] = decodeCacheFields(cacheResult, config.fieldTypes) as T; } } diff --git a/apps/event-engine/src/common/caches/imageData.cache.ts b/apps/event-engine/src/common/caches/imageData.cache.ts index 6a2fc6a501..b50665db28 100644 --- a/apps/event-engine/src/common/caches/imageData.cache.ts +++ b/apps/event-engine/src/common/caches/imageData.cache.ts @@ -23,6 +23,7 @@ const ALWAYS_INCLUDE_TAGS = ['anime', 'cartoon', 'comics', 'manga', 'man', 'woma export const imageTagIds = createCache({ redisKey: 'image:tagIds', idKey: 'imageId', + fieldTypes: { imageId: 'number', tags: 'json' }, async fetch(ctx: CacheContext, ids: number[]): Promise { // Fetch tags on image const imageTags = await ctx.pg.query<{ @@ -110,6 +111,8 @@ export type TagData = { export const tagData = createCache({ redisKey: 'tag:data', idKey: 'id', + // A tag NAME is a string even when it is all digits ('1', '2girls' etc.). + fieldTypes: { id: 'number', name: 'string', type: 'number', nsfwLevel: 'number' }, async fetch(ctx: CacheContext, ids: number[]): Promise { return await ctx.pg.query( `SELECT @@ -143,6 +146,13 @@ export type CosmeticData = { export const cosmeticData = createCache({ redisKey: 'cosmetic:data', idKey: 'id', + fieldTypes: { + id: 'number', + name: 'string', + type: 'string', + data: 'json', + source: 'string', + }, async fetch(ctx: CacheContext, ids: number[]): Promise { return await ctx.pg.query( `SELECT @@ -177,6 +187,7 @@ export type UserCosmeticData = { export const userCosmetics = createCache({ redisKey: 'user:cosmetics', idKey: 'userId', + fieldTypes: { userId: 'number', cosmetics: 'json' }, async fetch(ctx: CacheContext, ids: number[]): Promise { const cosmetics = await ctx.pg.query<{ userId: number; @@ -233,6 +244,17 @@ export type ProfilePictureData = { export const profilePictures = createCache({ redisKey: 'user:profilePicture', idKey: 'userId', + fieldTypes: { + userId: 'number', + id: 'number', + url: 'string', + nsfwLevel: 'number', + hash: 'string', + type: 'string', + width: 'number', + height: 'number', + metadata: 'json?', + }, async fetch(ctx: CacheContext, ids: number[]): Promise { return await ctx.pg.query( `SELECT diff --git a/apps/event-engine/src/common/caches/modelData.cache.ts b/apps/event-engine/src/common/caches/modelData.cache.ts index 335023bae6..95fb40fc19 100644 --- a/apps/event-engine/src/common/caches/modelData.cache.ts +++ b/apps/event-engine/src/common/caches/modelData.cache.ts @@ -16,6 +16,13 @@ export type ModelCacheData = { export const modelData = createCache({ redisKey: 'model:data', idKey: 'modelId', + fieldTypes: { + modelId: 'number', + name: 'string', + type: 'string', + nsfw: 'boolean', + userId: 'number', + }, async fetch({ pg }: CacheContext, ids: number[]) { const models = await pg.query( `SELECT diff --git a/apps/event-engine/src/common/caches/userData.cache.ts b/apps/event-engine/src/common/caches/userData.cache.ts index 7aa7604c49..f82e5c7446 100644 --- a/apps/event-engine/src/common/caches/userData.cache.ts +++ b/apps/event-engine/src/common/caches/userData.cache.ts @@ -15,6 +15,20 @@ export type UserCacheData = { export const userData = createCache({ redisKey: 'user:data', idKey: 'userId', + // 🔴 `username` is a STRING and must never be inferred. A Civitai username may + // be entirely digits (`usernameSchema` is `/^[A-Za-z0-9_]*$/`), and inferring + // the type from the stored text turned `'0222'` into `222` — a name that + // matches no account — and `'2428023993'` into an unquoted JSON number that + // no typed API client could decode. civitai#4768 / civitai/cli#513. + // + // `username` is NOT declared nullable, and that is deliberate: the column is + // `NOT NULL`, and `'null'` is a name this regex allows someone to hold. + fieldTypes: { + userId: 'number', + username: 'string', + image: 'string?', + deletedAt: 'date?', + }, async fetch({ pg }: CacheContext, ids: number[]) { const users = await pg.query( `SELECT diff --git a/apps/event-engine/src/common/caches/value-codec.ts b/apps/event-engine/src/common/caches/value-codec.ts new file mode 100644 index 0000000000..288bce03f3 --- /dev/null +++ b/apps/event-engine/src/common/caches/value-codec.ts @@ -0,0 +1,158 @@ +/** + * The ONE place that decides how a cached entity field is read back out of a + * Redis hash. + * + * A Redis hash stores strings and nothing else, so every field of every cache + * built by `createCache` is serialised on write and de-serialised on read. The + * read used to GUESS the original type back from the text: + * + * item[key] = isNaN(Number(value)) ? value : Number(value); + * + * That guess is lossy, and it is lossy for real, legal data: + * + * - `'0222'` -> `222` a username's leading zeros are DESTROYED, and + * the name that comes out matches no account + * - `'2428023993'` -> `2428023993` an all-digit username becomes a JSON + * number, so `/api/v1/images` emitted + * `"username":2428023993` unquoted and every + * typed client failed to decode the 200 + * - `''` -> `0` because `Number('') === 0` + * - `'false'` -> `'false'` a truthy string, for a stored `false` + * - `'null'` -> `'null'` likewise truthy, and `deletedAt` is read as + * `!!value` by several consumers + * + * See civitai#4768 / civitai/cli#513. A Civitai username may be entirely digits + * — `usernameSchema` is `/^[A-Za-z0-9_]*$/` — so this is legal data, not bad + * data, and the endpoint's legacy Prisma branch (which does not go through this + * cache) returned the same names correctly quoted the whole time. + * + * 🔴 THE FIX DELIBERATELY DOES NOT CHANGE THE STORED FORMAT. The bytes in Redis + * are already correct — `'0222'` is stored as `0222`; only the read destroyed + * it. Re-encoding (per-field JSON, say) would be tidier, but it cannot be rolled + * out without a window in which processes running the OLD code read entries + * written by the NEW code and render every string with literal quotes, and it + * would only start helping once each entry had been rewritten (up to its TTL). + * Declaring the field types instead is correct for entries ALREADY in Redis, on + * the first read, with no migration, no cold cache and no version skew. + * + * What a declaration cannot repair is a field whose stored text is genuinely + * ambiguous for its own declared type — `'null'` for a non-nullable string is + * indistinguishable from a user literally named `null`. Those are called out at + * the declaration sites. + */ + +/** + * How one cached field is to be read back. + * + * A trailing `?` marks the field nullable, which is what makes the stored text + * `'null'` decode to `null` instead of to the truthy four-character string. It + * is opt-in per field precisely because `'null'` is a legal username. + */ +export type CacheFieldType = + | 'string' + | 'string?' + | 'number' + | 'number?' + | 'boolean' + | 'boolean?' + | 'date' + | 'date?' + | 'json' + | 'json?'; + +/** + * The declared type of every field a cache stores. + * + * Non-`Partial` on purpose: this is a compiler-enforced ledger, so adding a + * column to a cache's `SELECT` (and to its `T`) fails to typecheck until the + * field is declared here. Without that, a new field silently falls back to the + * type guess this module exists to remove. + */ +export type CacheFieldTypes = Record, CacheFieldType>; + +/** + * Hash fields that carry cache bookkeeping rather than entity data, and so must + * never be decoded into the returned item. + */ +const RESERVED_FIELDS: ReadonlySet = new Set(['cachedAt', 'notFound', 'debounce']); + +/** + * De-serialise a Redis hash into an entity, using the cache's declared field + * types. Both read paths in `createCache` — the ordinary one and the + * lock-contention retry — go through here, so they cannot drift apart. + */ +export function decodeCacheFields( + hash: Record, + fieldTypes: CacheFieldTypes +): Record { + const item: Record = {}; + + for (const [key, value] of Object.entries(hash)) { + if (RESERVED_FIELDS.has(key)) continue; + + const declared = (fieldTypes as Record)[key]; + item[key] = declared ? decodeDeclared(value, declared) : decodeUndeclared(value); + } + + return item; +} + +function decodeDeclared(value: string, declared: CacheFieldType): unknown { + const nullable = declared.endsWith('?'); + const base = nullable ? declared.slice(0, -1) : declared; + + // `String(null)`/`String(undefined)` are what the writer stored for an absent + // value; both mean "no value" for a field declared nullable. + if (nullable && (value === 'null' || value === 'undefined')) return null; + + switch (base) { + case 'string': + return value; + case 'number': { + const asNumber = Number(value); + return Number.isNaN(asNumber) ? value : asNumber; + } + case 'boolean': + if (value === 'true') return true; + if (value === 'false') return false; + return value; + case 'date': { + const asDate = new Date(value); + return Number.isNaN(asDate.getTime()) ? value : asDate; + } + case 'json': + default: + return parseJson(value); + } +} + +/** + * Fallback for a hash field with no declaration — a stale field left behind by a + * removed column, or a bookkeeping key added by a future writer. + * + * This is the old guess with ONE repair, which is pure gain and cannot be wrong: + * a number is recovered only when the text is the CANONICAL rendering of that + * number, i.e. `String(Number(text)) === text`. `String(222)` can never produce + * the text `'0222'`, so `'0222'` was never a number and must not be read back as + * one. The same reasoning rescues `''` (`String(0)` is `'0'`, not `''`) and + * `'1e5'`. + */ +function decodeUndeclared(value: string): unknown { + if (value.startsWith('[') || value.startsWith('{')) { + const parsed = parseJson(value); + if (parsed !== value) return parsed; + } + + const asNumber = Number(value); + if (!Number.isNaN(asNumber) && String(asNumber) === value) return asNumber; + return value; +} + +/** JSON.parse that returns the raw text rather than throwing a whole read away. */ +function parseJson(value: string): unknown { + try { + return JSON.parse(value); + } catch { + return value; + } +} diff --git a/event-engine-common b/event-engine-common index e796afee3c..5da5cc6467 160000 --- a/event-engine-common +++ b/event-engine-common @@ -1 +1 @@ -Subproject commit e796afee3c9b8db1ab6c6dc3e9463614539ed7e0 +Subproject commit 5da5cc646717560f157bda79b8269a288e56e036 diff --git a/src/server/services/__tests__/feed-cache-value-codec.test.ts b/src/server/services/__tests__/feed-cache-value-codec.test.ts new file mode 100644 index 0000000000..1d3f54fe9a --- /dev/null +++ b/src/server/services/__tests__/feed-cache-value-codec.test.ts @@ -0,0 +1,317 @@ +import { describe, expect, it, vi } from 'vitest'; +import { createCache } from '../../../../event-engine-common/caches/base'; +import { userData } from '../../../../event-engine-common/caches/userData.cache'; +import type { + IClickhouseClient, + IRedisClient, +} from '../../../../event-engine-common/types/package-stubs'; + +/** + * REGRESSION GUARD for civitai#4768 (reported downstream as civitai/cli#513). + * + * `/api/v1/images?username=` emitted `username` as a bare JSON + * NUMBER, and for a name with a leading zero it emitted a DIFFERENT name: + * `0222` came back as `222`, which round-trips to no account at all. + * + * The coercion is NOT in Meilisearch and NOT at the endpoint. It is in the feed's + * Redis entity cache. A Redis hash stores only strings, so `createCache` + * serialises every field on write and de-serialises it on read — and the read + * used to GUESS the type back: + * + * item[key] = isNaN(Number(value)) ? value : Number(value); + * + * `username` is a string that can be all digits (`usernameSchema` is + * `/^[A-Za-z0-9_]*$/`), so this is legal data, not bad data. The REST endpoint's + * legacy Prisma branch never goes through this cache, which is exactly why + * `?imageId=` was quoted while `?username=` was not. + * + * These tests drive the REAL `createCache` against a fake Redis and assert the + * property that matters: whatever the fetcher produced is what a cache HIT + * returns. Fixture values are pairwise distinct so no assertion can pass by + * collapsing onto a neighbour's value or onto a constant it names itself. + */ + +type Row = { + userId: number; + username: string; + altName: string; + bio: string; + nsfw: boolean; + deletedAt: string | null; + image: string; + tags: number[]; + meta: { weight: number }; +}; + +/** A user whose real name is all digits WITH a leading zero — the worst case. */ +const ROW: Row = { + userId: 4768, + username: '0222', + altName: '2428023993', + bio: '', + nsfw: false, + deletedAt: null, + image: 'avatar-7f3a', + tags: [3, 9], + meta: { weight: 51 }, +}; + +const FIELD_TYPES = { + userId: 'number', + username: 'string', + altName: 'string', + bio: 'string', + nsfw: 'boolean', + deletedAt: 'date?', + image: 'string', + tags: 'json', + meta: 'json', +} as const; + +type FakeRedis = IRedisClient & { + __hashes: Map>; + __strings: Map; + __hGetAllCalls: number; +}; + +function makeFakeRedis(opts: { hideFirstRead?: boolean } = {}): FakeRedis { + const hashes = new Map>(); + const strings = new Map(); + const client = { + __hashes: hashes, + __strings: strings, + __hGetAllCalls: 0, + async hGetAll(key: string) { + client.__hGetAllCalls += 1; + if (opts.hideFirstRead && client.__hGetAllCalls === 1) return {}; + return { ...(hashes.get(key) ?? {}) }; + }, + async hSet(key: string, fields: Record) { + const current = hashes.get(key) ?? {}; + Object.assign(current, fields); + hashes.set(key, current); + return 1; + }, + async expire() { + return 1; + }, + async set(key: string, value: string, options?: { NX?: boolean; EX?: number }) { + if (options?.NX && strings.has(key)) return null; + strings.set(key, value); + return 'OK'; + }, + async del(keys: string | string[]) { + const list = Array.isArray(keys) ? keys : [keys]; + for (const key of list) { + strings.delete(key); + hashes.delete(key); + } + return list.length; + }, + } as unknown as FakeRedis; + return client; +} + +const noopPg = { query: async () => [] } as unknown as { + query: (q: string, p?: any[]) => Promise; +}; +const noopCh = noopPg as unknown as IClickhouseClient & typeof noopPg; + +function makeCache(rows: Row[], redisKey: string) { + const fetcher = vi.fn(async () => rows); + const cache = createCache({ + redisKey, + idKey: 'userId', + fieldTypes: FIELD_TYPES, + fetch: fetcher, + }); + return { cache, fetcher }; +} + +function ctxFor(redis: FakeRedis) { + return { redis, pg: noopPg, ch: noopCh } as any; +} + +describe('createCache value codec — civitai#4768 numeric-username coercion', () => { + it('POSITIVE CONTROL: the second read is served from Redis, so the decode path really runs', async () => { + const redis = makeFakeRedis(); + const { cache, fetcher } = makeCache([ROW], 'codec:control'); + + const miss = await cache.fetch(ctxFor(redis), [ROW.userId]); + expect(fetcher).toHaveBeenCalledTimes(1); + // The miss returns the fetcher's own object untouched — it proves nothing + // about the codec, which is why every assertion below reads the SECOND call. + expect(miss[ROW.userId].username).toBe('0222'); + + // Something was actually written to Redis. Without this, a cache that stored + // nothing would make every "hit" below a silent re-fetch and the whole file + // would pass while testing no decode at all. + expect(redis.__hashes.get(`codec:control:${ROW.userId}`)).toBeDefined(); + + const hit = await cache.fetch(ctxFor(redis), [ROW.userId]); + expect(fetcher).toHaveBeenCalledTimes(1); // no second fetch => served from cache + expect(hit[ROW.userId]).toBeDefined(); + }); + + it('keeps an all-digit username with a leading zero as the SAME string', async () => { + const redis = makeFakeRedis(); + const { cache } = makeCache([ROW], 'codec:leading-zero'); + + await cache.fetch(ctxFor(redis), [ROW.userId]); + const hit = await cache.fetch(ctxFor(redis), [ROW.userId]); + + expect(hit[ROW.userId].username).toBe('0222'); + expect(typeof hit[ROW.userId].username).toBe('string'); + }); + + it('keeps an all-digit username WITHOUT a leading zero as a string, not a number', async () => { + const redis = makeFakeRedis(); + const { cache } = makeCache([ROW], 'codec:all-digit'); + + await cache.fetch(ctxFor(redis), [ROW.userId]); + const hit = await cache.fetch(ctxFor(redis), [ROW.userId]); + + expect(hit[ROW.userId].altName).toBe('2428023993'); + expect(typeof hit[ROW.userId].altName).toBe('string'); + }); + + it('round-trips every field type the caches actually store', async () => { + const redis = makeFakeRedis(); + const { cache } = makeCache([ROW], 'codec:round-trip'); + + await cache.fetch(ctxFor(redis), [ROW.userId]); + const hit = await cache.fetch(ctxFor(redis), [ROW.userId]); + + // Numbers stay numbers (INVARIANT GUARD — this held before the fix too; it + // is here so an over-correction that stringified everything is caught). + expect(hit[ROW.userId].userId).toBe(4768); + // `false` used to come back as the STRING 'false', which is truthy. + expect(hit[ROW.userId].nsfw).toBe(false); + // `null` used to come back as the STRING 'null', which is truthy — and + // `deletedAt` is branched on with `!!user.deletedAt` by several consumers. + expect(hit[ROW.userId].deletedAt).toBeNull(); + // An empty string used to come back as the NUMBER 0, because Number('') is 0. + expect(hit[ROW.userId].bio).toBe(''); + // Non-numeric strings were never affected (INVARIANT GUARD). + expect(hit[ROW.userId].image).toBe('avatar-7f3a'); + // Arrays/objects round-trip (INVARIANT GUARD for the uncontended read path). + expect(hit[ROW.userId].tags).toEqual([3, 9]); + expect(hit[ROW.userId].meta).toEqual({ weight: 51 }); + }); + + it('decodes arrays on the CONTENDED read path, not just the uncontended one', async () => { + // The lock-contention retry loop had its OWN copy of the decoder, and that + // copy lacked the array/object branch entirely — so a cached `tags` array + // came back as the raw string '[3,9]' and every `Array.isArray` check on it + // silently saw `false`. Consolidating the two decoders is what fixes this. + const redis = makeFakeRedis({ hideFirstRead: true }); + const key = 'codec:contended'; + const { cache, fetcher } = makeCache([ROW], key); + + // Warm the hash the way a real writer would (through the cache itself), then + // hand the next reader a HELD lock so it takes the retry path. + const warm = makeFakeRedis(); + await cache.fetch(ctxFor(warm), [ROW.userId]); + const stored = warm.__hashes.get(`${key}:${ROW.userId}`); + expect(stored).toBeDefined(); + redis.__hashes.set(`${key}:${ROW.userId}`, { ...(stored as Record) }); + redis.__strings.set(`lock:${key}:${ROW.userId}`, '1'); // someone else holds it + + fetcher.mockClear(); + const hit = await cache.fetch(ctxFor(redis), [ROW.userId]); + + expect(fetcher).not.toHaveBeenCalled(); // proves the retry path served it + expect(hit[ROW.userId].tags).toEqual([3, 9]); + expect(hit[ROW.userId].username).toBe('0222'); + expect(hit[ROW.userId].meta).toEqual({ weight: 51 }); + }); + + it('repairs entries ALREADY in Redis, with no re-write and no migration', async () => { + // The stored bytes were never wrong — `0222` is in the hash as `0222`; only + // the read destroyed it. This is why the fix declares field types instead of + // re-encoding: an entry written by a process running the OLD code decodes + // correctly on the FIRST read, so there is no TTL to wait out, no cold cache + // and no window in which two deployed versions disagree about the format. + const redis = makeFakeRedis(); + const key = 'codec:pre-existing'; + const { cache, fetcher } = makeCache([ROW], key); + + redis.__hashes.set(`${key}:${ROW.userId}`, { + cachedAt: new Date().toISOString(), + userId: '4768', + username: '0222', + altName: '2428023993', + bio: '', + nsfw: 'false', + deletedAt: 'null', + image: 'avatar-7f3a', + tags: '[3,9]', + meta: '{"weight":51}', + }); + + const hit = await cache.fetch(ctxFor(redis), [ROW.userId]); + expect(fetcher).not.toHaveBeenCalled(); // decoded straight out of the old hash + + expect(hit[ROW.userId].username).toBe('0222'); + expect(hit[ROW.userId].altName).toBe('2428023993'); + expect(hit[ROW.userId].userId).toBe(4768); + expect(hit[ROW.userId].nsfw).toBe(false); + expect(hit[ROW.userId].deletedAt).toBeNull(); + expect(hit[ROW.userId].bio).toBe(''); + expect(hit[ROW.userId].tags).toEqual([3, 9]); + }); + + it('a field with no declaration no longer destroys leading zeros', async () => { + // Fallback path: a stale hash field left behind by a removed column has no + // declaration. It still gets the type guess, but only when the text is the + // CANONICAL rendering of the number — `String(222)` can never produce the + // text '0222', so '0222' was never a number. + const redis = makeFakeRedis(); + const key = 'codec:undeclared'; + const { cache, fetcher } = makeCache([ROW], key); + + redis.__hashes.set(`${key}:${ROW.userId}`, { + cachedAt: new Date().toISOString(), + userId: '4768', + username: '0222', + altName: '2428023993', + bio: '', + nsfw: 'false', + deletedAt: 'null', + image: 'avatar-7f3a', + tags: '[3,9]', + meta: '{"weight":51}', + legacyPaddedCode: '0077', // no declaration + legacyCount: '42', // no declaration + }); + + const hit = (await cache.fetch(ctxFor(redis), [ROW.userId]))[ROW.userId] as unknown as Record< + string, + unknown + >; + expect(fetcher).not.toHaveBeenCalled(); + + expect(hit.legacyPaddedCode).toBe('0077'); + // A canonical number is still recovered as one (INVARIANT GUARD). + expect(hit.legacyCount).toBe(42); + }); +}); + +describe('the userData cache declares username as a string', () => { + // A BEHAVIOURAL check on the real cache object, not a spelling check: the + // shipped `userData` config is what the images feed uses to build + // `user.username`, and it is the one declaration whose absence caused #4768. + it('never infers a type for username', async () => { + const redis = makeFakeRedis(); + const rows = [{ userId: 4768, username: '0222', image: null, deletedAt: null }]; + const pg = { query: vi.fn(async () => rows) } as any; + const ctx = { redis, pg, ch: noopCh } as any; + + await userData.fetch(ctx, [4768]); + const hit = await userData.fetch(ctx, [4768]); + + expect(pg.query).toHaveBeenCalledTimes(1); // second read came from Redis + expect(hit[4768].username).toBe('0222'); + expect(typeof hit[4768].username).toBe('string'); + }); +}); diff --git a/src/server/services/__tests__/image-search-username-type.test.ts b/src/server/services/__tests__/image-search-username-type.test.ts new file mode 100644 index 0000000000..9fe92358fc --- /dev/null +++ b/src/server/services/__tests__/image-search-username-type.test.ts @@ -0,0 +1,130 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { NextApiRequest } from 'next'; + +/** + * `/api/v1/images` publishes `username` as a STRING. civitai#4768 / + * civitai/cli#513: it emitted a bare JSON number for an all-digit username, and + * an external Go client failed to decode the entire 200 with + * `cannot unmarshal number into Go struct field .items.username of type string`. + * + * The CAUSE is upstream — the feed's Redis entity cache inferred the field's + * type from its stored text — and is guarded by + * `feed-cache-value-codec.test.ts`. This file guards the OTHER half: the + * published wire contract, at the one place that builds it. + * + * 🔴 It is a belt, not the fix, and the third case below says so as an + * assertion rather than as a comment: a number that has already lost its leading + * zeros casts to a string that is still the WRONG NAME. A guard that only + * checked `typeof === 'string'` would call that a pass. + */ + +vi.mock('~/server/services/feature-flags.service', () => ({ + getFeatureFlags: vi.fn(() => ({ canViewNsfw: true, datapacketRead: false })), + buildFliptContext: vi.fn(() => ({})), +})); +vi.mock('~/server/flipt/client', () => ({ + FLIPT_FEATURE_FLAGS: {}, + getFliptVariant: vi.fn(async () => 'off'), +})); +vi.mock('~/server/redis/caches', () => ({ + imageMetaCache: { fetch: vi.fn(async () => ({})) }, +})); +vi.mock('~/client-utils/edge-url', () => ({ getEdgeUrl: vi.fn(() => 'https://edge/x') })); + +const feedSearch = vi.fn(async () => ({ items: [] as unknown[], nextCursor: undefined })); +const legacySearch = vi.fn(async () => ({ items: [] as unknown[], nextCursor: undefined })); +vi.mock('~/server/services/image.service', () => ({ + getAllImages: (...args: unknown[]) => legacySearch(...(args as [])), + getAllImagesIndex: vi.fn(async () => ({ items: [], nextCursor: undefined })), + getImagesFromFeedSearch: (...args: unknown[]) => feedSearch(...(args as [])), +})); + +import { runImageSearch } from '../image-search.service'; + +const req = { + headers: { 'user-agent': 'probe/1.0' }, + socket: { remoteAddress: '203.0.113.7' }, +} as unknown as NextApiRequest; + +function itemWithUsername(username: unknown) { + return { + id: 1446527, + url: 'abc', + hash: 'h', + width: 512, + height: 768, + nsfwLevel: 1, + type: 'image', + createdAt: new Date(0), + postId: 369748, + stats: {}, + user: { id: 4768, username }, + baseModel: 'SD 1.5', + modelVersionIds: [], + tags: [], + }; +} + +/** Drive the REAL service over the feed (non-legacy) branch. */ +async function usernameFromFeed(raw: unknown) { + feedSearch.mockResolvedValue({ items: [itemWithUsername(raw)], nextCursor: undefined }); + const { items } = await runImageSearch( + { limit: 1, withMeta: false, withTags: false, data: {} } as never, + { browsingLevel: 1, user: undefined, req } as never + ); + return items[0].username; +} + +/** Drive the REAL service over the legacy (`?imageId=`) branch. */ +async function usernameFromLegacy(raw: unknown) { + legacySearch.mockResolvedValue({ items: [itemWithUsername(raw)], nextCursor: undefined }); + const { items } = await runImageSearch( + { limit: 1, withMeta: false, withTags: false, data: { imageId: 1446527 } } as never, + { browsingLevel: 1, user: undefined, req } as never + ); + return items[0].username; +} + +describe('runImageSearch: the published type of `username`', () => { + beforeEach(() => { + feedSearch.mockClear(); + legacySearch.mockClear(); + }); + + it('POSITIVE CONTROL: the harness reaches both branches and can tell them apart', async () => { + await usernameFromFeed('alice'); + expect(feedSearch).toHaveBeenCalledTimes(1); + expect(legacySearch).not.toHaveBeenCalled(); + + await usernameFromLegacy('bob'); + expect(legacySearch).toHaveBeenCalledTimes(1); + expect(feedSearch).toHaveBeenCalledTimes(1); + }); + + it('emits a string when the feed hands it a number', async () => { + const username = await usernameFromFeed(2428023993); + expect(username).toBe('2428023993'); + expect(typeof username).toBe('string'); + }); + + it('CANNOT restore a name the cache already destroyed — this belt is not the fix', async () => { + // `0222` reached the wire as `222` because the cache decoded the stored text + // as a number. Casting at this boundary produces a well-typed 200 carrying a + // name that matches no account, which is why the cache fix is the real one. + const username = await usernameFromFeed(222); + expect(username).toBe('222'); + expect(username).not.toBe('0222'); + }); + + it('leaves an ordinary string untouched', async () => { + expect(await usernameFromFeed('0222')).toBe('0222'); + expect(await usernameFromLegacy('2428023993')).toBe('2428023993'); + }); + + it('does not turn a null username into the string "null"', async () => { + // The legacy Prisma branch can carry a null username for a deleted account. + // A bare `String(...)` here would publish `'null'` as somebody's name. + expect(await usernameFromLegacy(null)).toBeNull(); + expect(await usernameFromFeed(undefined)).toBeUndefined(); + }); +}); diff --git a/src/server/services/image-search.service.ts b/src/server/services/image-search.service.ts index 749179f082..624ee64fdc 100644 --- a/src/server/services/image-search.service.ts +++ b/src/server/services/image-search.service.ts @@ -252,7 +252,24 @@ export async function runImageSearch( const useFlat = flatMeta !== undefined ? flatMeta : !useLegacyMethod; return useFlat ? imageMeta : { id: image.id, meta: imageMeta }; })(), - username: image.user.username, + // `/api/v1/images` publishes `username` as a STRING, and a typed client + // fails to decode the whole 200 when it is not one — which is how #4768 + // was reported (civitai/cli#513: "cannot unmarshal number into Go struct + // field .items.username of type string"). + // + // The real repair is upstream, in the feed's entity cache, which used to + // infer a field's type from its stored text and so read the all-digit + // username `0222` back as the number `222` + // (`event-engine-common/caches/value-codec.ts`). This is a belt on the + // published contract, nothing more: it can keep a number from reaching the + // wire, but it CANNOT recover a name the cache already lost — `222` casts + // to `'222'`, which is still not `'0222'`. Do not read it as the fix. + // + // Only a `number` is coerced. `String()` on its own would be a regression: + // the legacy DB branch can carry a null username for a deleted account, + // and `String(null)` is the four-character string `'null'`. + username: + typeof image.user.username === 'number' ? String(image.user.username) : image.user.username, baseModel: image.baseModel, modelVersionIds: image.modelVersionIds, tags: withTags ? image.tags?.map((t) => ({ id: t.id, name: t.name })) ?? [] : undefined,