fix(tests): unbreak main — the redis-key guard and the reaction.toggle contract (#4544)

* test(preview): pin reaction.toggle to its restored awaited contract `preview / smoke-tests` has been red on main since #4516. The spec asserted `reaction.toggle` resolves to null; it now resolves to "created". The code is right and the assertion was stale. Timeline: f8756fb5dd and earlier .mutation(toggleReactionHandler) -> returns the result ee376ea7d8 (2026-03) perf: fire-and-forget -> returns undefined/null 1e810875c8 (#4516) revert to awaited -> returns the result So #4516 restored the pre-March contract rather than inventing a new one. It is deliberate on every available signal: the PR body says "Add regression test confirming the mutation returns the handler result", the router carries a "Must stay awaited" comment, and reaction.router.awaited.test.ts pins `expect(result).toBe('created')`. Reverting the router would reintroduce the bug #4516 fixed (the write detached from the request lifecycle, so a reaction could be dropped on pod drain and not survive a refresh). Nothing in production consumes the return value, so this is contained to test code. Both client call sites -- ReactionButton.tsx and Questions/FavoriteBadge.tsx -- destructure only `mutate`/`isPending` with no onSuccess/onSettled/onError and no mutateAsync; UI state is optimistic via zustand/useState. The procedure is not exposed through any REST bridge, the moderator OpenAPI catalog, or the tier-1 public-route catalog, so no published contract names its shape. Pins the exact discriminator rather than loosening to truthiness: the handler can also resolve "removed" or "noop", and both are non-null while meaning the reaction was NOT created -- which is the regression this spec exists to catch. The post is self-seeded in the same test and tester has never reacted to it, so the create branch is deterministic. Also rewrites the header block, which documented the fire-and-forget shape as current. * test(redis): take the queues integration mock off hand-typed key constants `Unit tests (1)` and `(2)` have been red on main since 54466eb231, which added src/server/redis/__tests__/queues.integration.test.ts hand-typing REDIS_SYS_KEYS and REDIS_SUB_KEYS inside its `~/server/redis/client` mock: REDIS_SYS_KEYS: { QUEUES: { BUCKETS: `${NS}:buckets` } }, REDIS_SUB_KEYS: { QUEUES: { MERGING: 'merging' } }, The rule is right and the file was wrong. no-hand-typed-redis-key-constants exists to guard exactly this, and it applies to test files by construction -- its glob is `**/*.test.{ts,tsx}` and nothing else, so "it should not apply to __tests__/" would empty the rule rather than narrow it. Its rationale documents 15 constants across 6 files that had already drifted from production unnoticed (#4400), including 'session:user-tokens' for 'session:user-tokens2'. MERGING was a live instance of that shape: hand-typed identical to production today, silently free to drift tomorrow. So no exemption, no suppression comment, and no baseline entry -- the list may only shrink and it does not move here. The one real constraint is that BUCKETS could not simply be the production value: every key the suite writes derives from it, and the file promises to touch nothing real on whatever Redis it is pointed at. Resolved by moving the isolation off the CONSTANT and onto the transport. The mock now spreads @civitai/redis/client for every key, and the sysRedis stub prefixes arg 0 of each command with the run-unique namespace. That mirrors what the Postgres half of this suite already does -- a scratch schema on the session search_path, with the SQL left unqualified -- and it means the suite now exercises the production key names instead of a copy of them. Verified against a real Postgres + Redis (the suite skips without both URLs, so CI never ran it): before 7 passed -- guard red: +queues.integration.test.ts after 7 passed -- guard green: 4 passed, positive control included Confirmed by observation rather than by the suite going green: redis MONITOR during a run shows HSET/SADD on "queues-it:<pid>:queues:buckets" and "queues-it:<pid>:queues:buckets:images_v6:Delete:<ts>", with the bucket name stored as an unprefixed production-shaped value, and cleanup's KEYS "queues-it:<pid>*" reaping both. Mutation-tested so the rewrite is not passing vacuously: - nsKey -> identity 2 tests fail on their own arrayContaining - drop the `...actual` spread 3 tests fail; proves the constants come from the package and NOT from the global src/__tests__/setup.ts mock Also checked: with both env vars unset the suite still skips cleanly (7 skipped, not 0 collected), so the added module-level import does not disturb the CI path. * test(redis): drive the canonical shared mocks from the queues integration suite Fixes the OTHER half of the red unit shards. `Unit tests (1)` and `(2)` are NOT the same failure -- they are two different guards tripped by the same file from 54466eb231: Unit tests (1) no-hand-typed-redis-key-constants hand-typed REDIS_*_KEYS Unit tests (2) no-direct-shared-module-mock direct vi.mock of a canonical specifier The previous commit fixed only the first. This fixes both, and supersedes its mechanism for the same file. queues.integration.test.ts directly mocked all three CANONICAL specifiers -- ~/server/db/client, ~/server/redis/client and ~/server/logging/client. Under `isolate: false` a per-file vi.mock of one of those freezes that shape into every later file in the same worker, which is the whole reason the guard exists. There is no exemption available and I did not try to manufacture one: scripts/test-perf/gen-mock-allowlist.mjs refuses on MEMBERSHIP growth (178 -> 179, exit 1), so the allowlist is not a route for a new file. Being an integration test is not an exemption either -- only a constraint on how the seam gets used. The canonical nodes are vi.fn()s, so rather than declaring canned return values this file now points them at the REAL clients it builds in beforeAll: dbMock.dbWrite.$queryRaw / $executeRaw -> the real scratch-schema PrismaClient, forwarding (strings, ...values) so the tagged-template binding this suite exists to prove is still exercised redisMock.sysRedis.<cmd> -> the real redis client, prefixing arg 0 with the run-unique namespace That also subsumes the hand-typed constants: setup.ts spreads the real @civitai/redis/client into the canonical factory, so REDIS_SYS_KEYS and REDIS_SUB_KEYS are production's own values and cannot drift -- no local override needed at all. withSysReadDeadline defaults to the real implementation there too, so queues.ts keeps running the real deadline wrapper. The local ~/server/logging/client stub is dropped because the canonical mock provides exactly logToAxiom. ~/server/redis/fail-open-log stays: it is a PENDING specifier, counted rather than enforced, with no canonical mock to move to. The generated allowlist is deliberately NOT regenerated here. It is not required -- canonical stays at 178 and the file no longer appears there -- and a regen sweeps in six unrelated pending-list entries that accumulated on main, which is churn this PR should not carry. Verified against a real Postgres + Redis (the suite skips without both URLs, so CI has never run it): 7 passed (7) before and after -- behaviour-preserving redis MONITOR identical wire traffic to the pre-rewrite version: HSET "queues-it:<pid>:queues:buckets" ... "queues:buckets:images_v6:Delete:<ts>" SADD "queues-it:<pid>:queues:buckets:images_v6:Delete:<ts>" "1" "2" "3" DEL both keys on cleanup Mutation-tested so the new wiring is not passing vacuously: - nsKey -> identity 2 tests fail on their own assertion - neuter the $queryRaw delegation 6 tests fail Both guards now pass, and the whole services suite that surfaced the second one goes 378 files / 5408 tests green (was 1 failed). Redis dir: 14 files, 124 tests. With both env vars unset the suite still skips cleanly (7 skipped, not 0 collected).
2026-09-20 22:08:18 +08:00 · 2026-09-01 10:57:38 -05:00
parent 0f2764458a
commit 9b04487b32
2 changed files with 99 additions and 72 deletions
@@ -1,8 +1,8 @@
 import { PrismaClient } from '@prisma/client';
+import { REDIS_SYS_KEYS } from '@civitai/redis/client';
 import { createClient } from 'redis';
 import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest';
-// Top-level, not an inline `typeof import(...)` — that trips consistent-type-imports.
-import type * as SysReadDeadline from '~/server/redis/sys-read-deadline';
+import { dbMock, redisMock } from '~/__tests__/mocks';

 /**
 * End-to-end proof for the dropped-enqueue parking lot, against a REAL Postgres and a
@@ -27,54 +27,39 @@ const redisUrl = process.env.QUEUES_IT_REDIS_URL;

 const SCHEMA = 'queues_it';

+const NS = `queues-it:${process.pid}`;
+
 // `backing = null` is the outage: every command rejects fast, which is the DOWN mode
 // queues.ts fails open on. Swapping it back is the recovery the drain has to survive.
-const { holder, NS } = vi.hoisted(() => ({
-  holder: { backing: null as null | Record<string, (...args: never[]) => Promise<unknown>> },
-  NS: `queues-it:${process.pid}`,
-}));
+let backing: typeof redis | null = null;

-vi.mock('~/server/redis/client', async () => {
-  const { withSysReadDeadline } = await vi.importActual<typeof SysReadDeadline>(
-    '~/server/redis/sys-read-deadline'
-  );
-  const call =
-    (fn: string) =>
-    (...args: never[]) =>
-      holder.backing
-        ? holder.backing[fn](...args)
-        : Promise.reject(new Error('sysRedis unavailable (test outage)'));
-  return {
-    sysRedis: {
-      hGet: call('hGet'),
-      hSet: call('hSet'),
-      sAdd: call('sAdd'),
-      sMembers: call('sMembers'),
-      del: call('del'),
-      exists: call('exists'),
-      set: call('set'),
-    },
-    REDIS_SYS_KEYS: { QUEUES: { BUCKETS: `${NS}:buckets` } },
-    REDIS_SUB_KEYS: { QUEUES: { MERGING: 'merging' } },
-    withSysReadDeadline,
-  };
-});
+/**
+ * 🔴 This file drives the CANONICAL shared mocks rather than registering its own.
+ *
+ * `~/server/db/client`, `~/server/redis/client` and `~/server/logging/client` all have
+ * canonical mocks registered once in `src/__tests__/setup.ts`, and a per-file `vi.mock` of
+ * any of them freezes that shape into every later file in the same worker under
+ * `isolate: false` — which is what `no-direct-shared-module-mock` exists to stop, and why
+ * `gen-mock-allowlist.mjs` refuses to allowlist a new file.
+ *
+ * Being an INTEGRATION test is not an exemption from that, only a constraint on how the
+ * seam is used: the canonical nodes are `vi.fn()`s, so instead of declaring canned return
+ * values this file points them at the REAL Postgres and Redis clients it builds in
+ * `beforeAll`. That satisfies the guard and keeps the suite end-to-end.
+ *
+ * It also removes the hand-typed key constants this file used to carry — `setup.ts` spreads
+ * the real `@civitai/redis/client` into the canonical factory, so `REDIS_SYS_KEYS` and
+ * `REDIS_SUB_KEYS` are production's own values and cannot drift
+ * (`no-hand-typed-redis-key-constants`). `withSysReadDeadline` likewise defaults to the real
+ * implementation there, so queues.ts keeps running the real deadline wrapper.
+ */

+// `~/server/redis/fail-open-log` is a PENDING specifier: no canonical mock exists yet, so a
+// direct mock is counted rather than enforced. Stubbed because the real logger is noise here.
 vi.mock('~/server/redis/fail-open-log', () => ({ logSysRedisFailOpen: vi.fn() }));
-vi.mock('~/server/logging/client', () => ({ logToAxiom: vi.fn(() => Promise.resolve()) }));

 let prisma: PrismaClient;
 let bootstrap: PrismaClient;
-// A getter, not a value: the client cannot exist until beforeAll has the URL, and
-// queues.ts captures the binding at import.
-vi.mock('~/server/db/client', () => ({
-  get dbWrite() {
-    return prisma;
-  },
-  get dbRead() {
-    return prisma;
-  },
-}));

 const { addToQueue, drainDroppedEnqueues } = await import('~/server/redis/queues');

@@ -100,6 +85,36 @@ describe.skipIf(!databaseUrl || !redisUrl)('queues parking lot — real Postgres

    redis = createClient({ url: redisUrl });
    await redis.connect();
+
+    // Point the canonical db mock at the REAL scratch-schema client. queues.ts calls these as
+    // TAGGED TEMPLATES, so the implementation forwards (strings, ...values) verbatim — which
+    // is exactly the binding this suite exists to prove (the `::jsonb` cast, and
+    // `jsonb_array_length` on what we actually store).
+    dbMock.dbWrite.$queryRaw.mockImplementation((sql: TemplateStringsArray, ...values: unknown[]) =>
+      prisma.$queryRaw(sql, ...values)
+    );
+    dbMock.dbWrite.$executeRaw.mockImplementation(
+      (sql: TemplateStringsArray, ...values: unknown[]) => prisma.$executeRaw(sql, ...values)
+    );
+
+    // Point the canonical sysRedis mock at the REAL redis. Every key this suite writes derives
+    // from BUCKETS, so prefixing arg 0 namespaces the whole run at the transport layer while
+    // leaving the PRODUCTION key names in play — the mirror of the scratch SCHEMA on the
+    // Postgres side, where the SQL stays unqualified and the isolation lives on the connection.
+    // `del` accepts an array of keys as well as one.
+    const nsKey = (key: unknown) =>
+      Array.isArray(key) ? key.map((k) => `${NS}:${k}`) : `${NS}:${key}`;
+    const COMMANDS = ['hGet', 'hSet', 'sAdd', 'sMembers', 'del', 'exists', 'set'] as const;
+    for (const cmd of COMMANDS) {
+      redisMock.sysRedis[cmd].mockImplementation((key: unknown, ...rest: unknown[]) =>
+        backing
+          ? (backing as unknown as Record<string, (...a: unknown[]) => Promise<unknown>>)[cmd](
+              nsKey(key),
+              ...rest
+            )
+          : Promise.reject(new Error('sysRedis unavailable (test outage)'))
+      );
+    }
  }, 30000);

  afterAll(async () => {
@@ -119,7 +134,7 @@ describe.skipIf(!databaseUrl || !redisUrl)('queues parking lot — real Postgres
    await prisma.$executeRawUnsafe(`TRUNCATE ${SCHEMA}."KeyValue"`);
    const keys = await redis.keys(`${NS}*`);
    if (keys.length) await redis.del(keys);
-    holder.backing = redis as never;
+    backing = redis;
  });

  const parked = () =>
@@ -127,9 +142,13 @@ describe.skipIf(!databaseUrl || !redisUrl)('queues parking lot — real Postgres
      `SELECT "key","value" FROM ${SCHEMA}."KeyValue" ORDER BY "key"`
    );

+  // Reads through the same `${NS}:` prefix the sysRedis stub applies, against the REAL
+  // key names. The bucket NAME stored in the hash is a value, not a key, so it is
+  // unprefixed on the way out and has to be prefixed again to be read back.
  const bucketMembers = async () => {
-    const bucket = await redis.hGet(`${NS}:buckets`, 'images_v6:Delete');
-    return bucket ? redis.sMembers(bucket) : [];
+    const bucketsKey = `${NS}:${REDIS_SYS_KEYS.QUEUES.BUCKETS}`;
+    const bucket = await redis.hGet(bucketsKey, 'images_v6:Delete');
+    return bucket ? redis.sMembers(`${NS}:${bucket}`) : [];
  };

  it('a healthy enqueue reaches redis and writes nothing to Postgres', async () => {
@@ -140,7 +159,7 @@ describe.skipIf(!databaseUrl || !redisUrl)('queues parking lot — real Postgres
  });

  it('an outage parks the ids in Postgres instead of losing them', async () => {
-    holder.backing = null;
+    backing = null;

    await expect(addToQueue('images_v6:Delete', [10, 11])).resolves.toBe(false);

@@ -148,7 +167,7 @@ describe.skipIf(!databaseUrl || !redisUrl)('queues parking lot — real Postgres
  });

  it('successive drops during one outage accumulate under the same key', async () => {
-    holder.backing = null;
+    backing = null;
    await addToQueue('images_v6:Delete', [10, 11]);
    await addToQueue('images_v6:Delete', [12]);

@@ -156,9 +175,9 @@ describe.skipIf(!databaseUrl || !redisUrl)('queues parking lot — real Postgres
  });

  it('the drain replays them into redis once the outage ends, and clears the row', async () => {
-    holder.backing = null;
+    backing = null;
    await addToQueue('images_v6:Delete', [10, 11]);
-    holder.backing = redis as never;
+    backing = redis;

    await expect(drainDroppedEnqueues()).resolves.toEqual({ keys: 1, replayed: 2, reparked: 0 });

@@ -167,22 +186,22 @@ describe.skipIf(!databaseUrl || !redisUrl)('queues parking lot — real Postgres
  });

  it('the drain leaves the row parked while the outage continues, and never duplicates it', async () => {
-    holder.backing = null;
+    backing = null;
    await addToQueue('images_v6:Delete', [10, 11]);

    await expect(drainDroppedEnqueues()).resolves.toEqual({ keys: 1, replayed: 0, reparked: 2 });
    // Still exactly the two ids — a re-park would have appended a second copy.
    expect((await parked())[0].value).toEqual([10, 11]);

-    holder.backing = redis as never;
+    backing = redis;
    await expect(drainDroppedEnqueues()).resolves.toEqual({ keys: 1, replayed: 2, reparked: 0 });
    expect(await parked()).toHaveLength(0);
  });

  it('a drop landing between the drain read and its delete is not swallowed', async () => {
-    holder.backing = null;
+    backing = null;
    await addToQueue('images_v6:Delete', [10, 11]);
-    holder.backing = redis as never;
+    backing = redis;

    // The interleaving the length guard exists for: the drain has read [10,11] and is
    // about to delete when another pod parks id 12. The delete must refuse the grown row.
@@ -22,13 +22,19 @@ import { trpcMutation, uniqueToken } from './preview-trpc';
 *  - reaction.toggle (reaction.router.ts:9 guardedProcedure .input(toggleReactionSchema))
 *    — toggleReactionSchema (reaction.schema.ts:37): { entityId: number;
 *    entityType: enum(reactableEntities incl. 'post'); reaction: enum(ReviewReactions) }.
- *    ReviewReactions.Like = 'Like' (enums.ts:355). The mutation is FIRE-AND-FORGET:
- *    the router calls toggleReactionHandler(...).catch(handleLogError) and returns
- *    nothing (reaction.router.ts:13-17), which superjson serializes as `json: null`
- *    over the wire (NOT undefined — confirmed against a live preview run). Success
- *    therefore = the call RESOLVES (the helper throws on any HTTP/tRPC error, so
- *    reaching past it means auth + rate-limit + handler-dispatch all accepted it);
- *    we assert the resolved value is nullish rather than a specific type.
+ *    ReviewReactions.Like = 'Like' (enums.ts:355). The mutation is AWAITED and
+ *    RESOLVES TO THE HANDLER RESULT: `.mutation(toggleReactionHandler)`
+ *    (reaction.router.ts:15) returns toggleReaction()'s discriminator, one of
+ *    'created' | 'removed' | 'noop' (reaction.service.ts:25,30 → controller :286).
+ *    This post is self-seeded in this same test and `tester` has never reacted to
+ *    it, so getReaction finds nothing and the create branch runs — the value is
+ *    deterministically 'created', which is what we pin.
+ *
+ *    🔴 It used to resolve to null, and this spec used to assert that. `ee376ea7d8`
+ *    (2026-03) made the router fire-and-forget for latency; #4516 / `1e810875c8`
+ *    reverted that because detaching the write from the request lifecycle dropped
+ *    reactions on pod drain. `src/server/routers/__tests__/reaction.router.awaited.test.ts`
+ *    is the unit-level contract test for the restored shape — keep the two in step.
 *  - commentv2.upsert (commentv2.router.ts:62 guardedProcedure .input(upsertCommentv2Schema))
 *    — upsertCommentv2Schema (commentv2.schema.ts) extends commentConnectorSchema
 *    ({ entityId: number; entityType: enum incl. 'post' }) with a non-empty sanitized
@@ -60,18 +66,20 @@ test.describe('tester self-seeds a post, reacts to it, and comments on it (mutat
    });
    expect(typeof post?.id, 'post.create should return a numeric post id').toBe('number');

-    // 2. React to that exact post. reaction.toggle is fire-and-forget: it resolves
-    // to null over the wire (superjson encodes the void return as json:null), so
-    // success = the call resolves without the helper throwing. A broken
-    // auth/rate-limit/dispatch path would surface as an HTTP/tRPC error here.
-    const reaction = await trpcMutation(page.request, 'reaction.toggle', {
-      entityId: post!.id,
-      entityType: 'post',
-      reaction: 'Like',
-    });
-    // `?? null` collapses null/undefined alike so the assertion is robust to the
-    // exact void encoding — the point is "it was accepted", not its serialized form.
-    expect(reaction ?? null, 'reaction.toggle should be accepted (resolve, not throw)').toBeNull();
+    // 2. React to that exact post. reaction.toggle is awaited and resolves to the
+    // handler's result, so this asserts the WRITE happened, not merely that the call
+    // was accepted: a first Like on a post nobody has reacted to takes the create
+    // branch and resolves to 'created'. A broken auth/rate-limit/dispatch path would
+    // surface as an HTTP/tRPC error before we get here.
+    const reaction = await trpcMutation<'created' | 'removed' | 'noop'>(
+      page.request,
+      'reaction.toggle',
+      { entityId: post!.id, entityType: 'post', reaction: 'Like' }
+    );
+    // Pinned to the exact discriminator rather than a truthiness check: 'noop' and
+    // 'removed' are also non-null, and both would mean the reaction did NOT get
+    // created — the regression this spec exists to catch.
+    expect(reaction, 'reaction.toggle should create the reaction and say so').toBe('created');

    // 3. Comment on that exact post. upsert returns the created comment row, so we
    // assert it came back with a numeric id and that our token survived sanitization