diff --git a/apps/server/src/core/ai-chat/ai-chat.service.ts b/apps/server/src/core/ai-chat/ai-chat.service.ts index dfa1bb75..14826bd6 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.ts @@ -758,6 +758,13 @@ export class AiChatService implements OnModuleInit { // or violate the page_id FK on insert (this runs after res.hijack(), so a // DB error would break the stream). const originPageId: string | null = openPageContext?.id ?? null; + // ORPHAN-ON-BEGIN-FAILURE tradeoff (#486, B3): the chat row is inserted + // HERE, before runHooks.begin below. If begin fails (e.g. a 503 / run-slot + // rejection) the turn aborts before the client is told this new chatId, so + // an empty chat is left behind and a retry mints ANOTHER one. We accept this + // over reordering: begin needs a chatId to bind the run to, and inserting + // the chat first keeps the id stable + the FK/history-join invariants above + // intact. Orphan empty chats are cheap and swept by normal chat cleanup. const chat = await this.aiChatRepo.insert({ creatorId: user.id, workspaceId: workspace.id, diff --git a/apps/server/src/core/ai-chat/public-share-chat.spec.ts b/apps/server/src/core/ai-chat/public-share-chat.spec.ts index f65058d9..40e4a7e6 100644 --- a/apps/server/src/core/ai-chat/public-share-chat.spec.ts +++ b/apps/server/src/core/ai-chat/public-share-chat.spec.ts @@ -808,7 +808,7 @@ describe('PublicShareChatToolsService share scoping', () => { }; await expect(getSharePage.execute({ pageId: 'p-outside' })).rejects.toThrow( - /not part of this published share/i, + /not available in this share/i, ); // The tool delegated the resolve to the canonical boundary with the // forShare-scoped shareId, and returned NO content for a non-resolving page. @@ -841,7 +841,7 @@ describe('PublicShareChatToolsService share scoping', () => { await expect( getSharePage.execute({ pageId: 'p-restricted' }), - ).rejects.toThrow(/not part of this published share/i); + ).rejects.toThrow(/not available in this share/i); // No content was ever sanitized/returned for the blocked page. expect(shareService.updatePublicAttachments).not.toHaveBeenCalled(); }); @@ -1003,7 +1003,7 @@ describe('public-share assistant boundary locks (red-team regression guards)', ( }; await expect( getSharePage.execute({ pageId: 'p-elsewhere' }), - ).rejects.toThrow(/not part of this published share/i); + ).rejects.toThrow(/not available in this share/i); // The forged share id is the scope the boundary re-derivation rejects against. expect(shareService.resolveReadableSharePage).toHaveBeenCalledWith( 'FORGED-SHARE', diff --git a/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.spec.ts b/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.spec.ts index bc80acd6..eadcd3a6 100644 --- a/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.spec.ts +++ b/apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.spec.ts @@ -224,7 +224,7 @@ describe('PublicShareChatToolsService.forShare', () => { (tools.getSharePage as unknown as ToolExec).execute({ pageId: 'page-1', }), - ).rejects.toThrow('That page is not part of this published share.'); + ).rejects.toThrow('The requested page is not available in this share.'); // No content is ever fetched/returned for a non-resolving page. expect(shareService.updatePublicAttachments).not.toHaveBeenCalled(); diff --git a/apps/server/src/integrations/health/redis.health.spec.ts b/apps/server/src/integrations/health/redis.health.spec.ts index 1fd59c26..daed278c 100644 --- a/apps/server/src/integrations/health/redis.health.spec.ts +++ b/apps/server/src/integrations/health/redis.health.spec.ts @@ -16,7 +16,72 @@ import type { EnvironmentService } from '../environment/environment.service'; * implementation (requireActual) — only the constructor is wrapped to COUNT the * real clients it creates, which is precisely the leaking resource. */ -const mockLiveClients: Array<{ status: string; disconnect: () => void }> = []; +import type { Redis } from 'ioredis'; + +const mockLiveClients: Redis[] = []; + +/** + * Fully tear a REAL ioredis client down so NO timer survives jest's 1s exit + * window (this suite must exit cleanly WITHOUT forceExit; see #382). + * + * `connector.disconnect()` arms a ~12s "force-destroy the stream" `setTimeout` + * that is cleared ONLY by the stream's 'close' event — but only when the + * connector still holds a stream. Two problem cases: + * - a LIVE/connecting socket: disconnect arms the timer and 'close' may lag + * past jest's window, so we destroy the socket to make 'close' fire NOW; + * - a client BETWEEN reconnect attempts to a dead port: the held socket is + * ALREADY destroyed (its 'close' fired long ago), so disconnect would arm a + * timer whose clearing 'close' can never come again. We drop that dead stream + * reference BEFORE disconnect so the doomed timer is never armed. + * `disconnect()` itself also clears ioredis' own reconnect backoff timer. + */ +type DrainableStream = { destroyed?: boolean; destroy?: () => void } | null; +type DrainableClient = { + removeAllListeners: (event: string) => void; + disconnect: () => void; + stream?: DrainableStream; + connector?: { stream?: DrainableStream }; +}; + +async function drainClient(client: Redis): Promise { + if (!client || client.status === 'end') return; + const c = client as unknown as DrainableClient; + c.removeAllListeners('error'); + + // Drop an already-dead held socket so disconnect() can't arm a timer whose + // clearing 'close' will never fire again. + if (c.connector?.stream && c.connector.stream.destroyed) { + c.connector.stream = null; + } + if (c.stream && c.stream.destroyed) { + c.stream = null; + } + + await new Promise((resolve) => { + let done = false; + const finish = () => { + if (done) return; + done = true; + resolve(); + }; + client.once('end', finish); + // reconnect=false (the default): stop the retry loop and close the socket. + client.disconnect(); + // Force any still-live socket closed NOW so the connector's stream-destroy + // timer clears inside jest's window instead of lagging behind a real 'close'. + if (c.stream && !c.stream.destroyed) { + c.stream.destroy?.(); + } + // Fallback for a client with no live stream to emit 'end' (unref'd so it + // can never itself hold the loop open). + const fallback = setTimeout(finish, 500); + (fallback as { unref?: () => void }).unref?.(); + }); +} + +async function drainAll(): Promise { + await Promise.all(mockLiveClients.map((c) => drainClient(c))); +} jest.mock('ioredis', () => { const actual = jest.requireActual('ioredis'); @@ -53,17 +118,12 @@ describe('RedisHealthIndicator handle leak (#486)', () => { indicator = new RedisHealthIndicator(indicatorService, environmentService); }); - afterEach(() => { + afterEach(async () => { + // Drain (destroy socket + AWAIT 'end') every client the test created FIRST, + // so each is fully 'end' before onModuleDestroy's disconnect runs — that way + // no ioredis reconnect / stream-destroy timer outlives jest's exit window. + await drainAll(); indicator.onModuleDestroy(); - // Belt-and-braces: tear down anything the test created so ioredis reconnect - // timers do not keep the jest worker alive. - for (const c of mockLiveClients) { - try { - c.disconnect(); - } catch { - /* already gone */ - } - } }); it('creates exactly ONE Redis client across many probes while Redis is DOWN', async () => { @@ -93,3 +153,59 @@ describe('RedisHealthIndicator handle leak (#486)', () => { expect(mockLiveClients).toHaveLength(2); }); }); + +/** + * Happy-path regression guard (#486, B2): the FIRST probe against a LIVE Redis + * must report UP. + * + * With `lazyConnect: true` + `enableOfflineQueue: false`, a freshly-built client + * is in the `wait` state and the socket opens lazily. If the very first `ping()` + * is issued before an explicit `connect()`, ioredis rejects it instantly with + * "Stream isn't writeable and enableOfflineQueue options is false" — a FALSE + * DOWN even though Redis is alive. The fix opens the socket before the first + * ping. This exercises a REAL ioredis client against a REAL TCP redis server + * (not a mock), so a regression genuinely reddens it. + */ +describe('RedisHealthIndicator live Redis first-probe (#486, B2)', () => { + const indicatorService = { + check: (key: string) => ({ + up: () => ({ [key]: { status: 'up' } }), + down: (message: string) => ({ [key]: { status: 'down', message } }), + }), + } as unknown as HealthIndicatorService; + + // A REAL running redis (see the neighboring harness / CI env). + const environmentService = { + getRedisUrl: () => 'redis://127.0.0.1:6379/0', + } as unknown as EnvironmentService; + + let indicator: RedisHealthIndicator; + + beforeEach(() => { + mockLiveClients.length = 0; + indicator = new RedisHealthIndicator(indicatorService, environmentService); + }); + + afterEach(async () => { + // Await full socket close of every live client (see drainClient) BEFORE + // onModuleDestroy: a real, connected ioredis client MUST be drained to 'end' + // or its stream-destroy timer keeps the jest worker alive past the 1s window. + await drainAll(); + indicator.onModuleDestroy(); + }); + + it('reports UP on the FIRST probe against a live Redis', async () => { + // The VERY FIRST probe — no warm-up ping — must be UP. + const result = await indicator.pingCheck('redis'); + expect(result.redis.status).toBe('up'); + }); + + it('stays UP on a probe AFTER onModuleDestroy re-creates the client', async () => { + await indicator.pingCheck('redis'); + indicator.onModuleDestroy(); + // The re-created client is again in `wait`; the first ping on it must still + // open the socket (the false-DOWN also recurs on the post-destroy path). + const result = await indicator.pingCheck('redis'); + expect(result.redis.status).toBe('up'); + }); +}); diff --git a/apps/server/src/integrations/health/redis.health.ts b/apps/server/src/integrations/health/redis.health.ts index c9160125..324d5a64 100644 --- a/apps/server/src/integrations/health/redis.health.ts +++ b/apps/server/src/integrations/health/redis.health.ts @@ -20,6 +20,26 @@ export class RedisHealthIndicator implements OnModuleDestroy { */ private probeClient: Redis | null = null; + /** + * How long the first-ping `connect()` may take before a probe gives up and + * reports DOWN. A `connect()` against a truly-down Redis never settles on its + * own (ioredis retries the socket indefinitely per its retryStrategy), so the + * probe MUST bound it or the /health handler would hang. Kept short so a real + * outage is reported fast; localhost/live Redis connects well within it. + */ + private static readonly CONNECT_TIMEOUT_MS = 2000; + + /** + * The single in-flight first-`connect()`, memoized so CONCURRENT probes share + * it. k8s liveness+readiness hit /health in parallel on startup: without this, + * probe A drives `connect()` (the client leaves the `wait` state) and probe B, + * seeing a not-`wait`/not-`ready` client, would skip connect and fire `ping()` + * at a still-opening socket → an instant FALSE DOWN. With the memo, B awaits + * the SAME connect. Cleared once it settles so a later disconnect / re-create + * starts a fresh connect. + */ + private connectingPromise: Promise | null = null; + constructor( private readonly healthIndicatorService: HealthIndicatorService, private environmentService: EnvironmentService, @@ -52,11 +72,82 @@ export class RedisHealthIndicator implements OnModuleDestroy { return this.probeClient; } + /** + * Open the probe socket BEFORE the first ping. `lazyConnect: true` leaves a + * freshly-built (or post-destroy re-built) client in the `wait` state: the + * socket is NOT open yet, so with `enableOfflineQueue: false` the very first + * `ping()` rejects instantly with "Stream isn't writeable and + * enableOfflineQueue options is false" even when Redis is perfectly alive — a + * false DOWN on the happy path. We drive `connect()` ONLY from `wait`; once + * the client is connected, ioredis owns its own (re)connect loop and a ping + * issued while it reconnects still fast-fails to a correct DOWN (offline queue + * stays off). A failed/timed-out connect rejects → reported DOWN, which is the + * right signal for a truly-down Redis. + */ + private ensureConnected(client: Redis): Promise { + // Already open — steady state, nothing to do. + if (client.status === 'ready') return Promise.resolve(); + // A first-connect is already in flight (possibly started by a CONCURRENT + // probe): await the SAME one instead of racing a second connect() (ioredis + // throws "already connecting") or firing ping() at a not-yet-open socket. + if (this.connectingPromise) return this.connectingPromise; + // Only DRIVE connect() from the initial `wait` state (fresh / post-destroy + // re-created client). In any other non-ready state ioredis already owns its + // (re)connect loop; a ping there fast-fails to a correct DOWN, so we must not + // start a competing connect. + if (client.status !== 'wait') return Promise.resolve(); + + const promise = this.connectWithTimeout(client).finally(() => { + // Clear only if still ours, so a later disconnect / re-create can connect + // again. Whether it resolved or rejected, the memo has served its window. + if (this.connectingPromise === promise) { + this.connectingPromise = null; + } + }); + this.connectingPromise = promise; + return promise; + } + + private connectWithTimeout(client: Redis): Promise { + return new Promise((resolve, reject) => { + let settled = false; + const timer = setTimeout(() => { + if (settled) return; + settled = true; + reject(new Error('Redis probe connect timed out')); + }, RedisHealthIndicator.CONNECT_TIMEOUT_MS); + // Never let THIS timer alone keep the event loop (or a jest worker) alive; + // it is cleared on settle anyway, this is belt-and-braces. + timer.unref?.(); + // `.catch` is always attached, so a connect() that rejects AFTER we have + // already timed out is handled here (guarded by `settled`) and never + // surfaces as an unhandled rejection. + client + .connect() + .then(() => { + if (settled) return; + settled = true; + clearTimeout(timer); + resolve(); + }) + .catch((err) => { + if (settled) return; + settled = true; + clearTimeout(timer); + reject(err); + }); + }); + } + async pingCheck(key: string): Promise { const indicator = this.healthIndicatorService.check(key); try { const redis = this.getProbeClient(); + // Open the socket before the first ping (see ensureConnected); without + // this the first probe after (re)creation falsely reports DOWN on a live + // Redis because lazyConnect defers the connect past the first ping. + await this.ensureConnected(redis); await redis.ping(); return indicator.up(); } catch (e) { @@ -75,5 +166,9 @@ export class RedisHealthIndicator implements OnModuleDestroy { this.probeClient.disconnect(); this.probeClient = null; } + // Drop any in-flight first-connect memo so the NEXT client (lazily rebuilt on + // the next probe) starts a fresh connect rather than awaiting a promise tied + // to the client we just tore down. + this.connectingPromise = null; } } diff --git a/packages/mcp/src/tool-specs.ts b/packages/mcp/src/tool-specs.ts index 44534ed1..941eead9 100644 --- a/packages/mcp/src/tool-specs.ts +++ b/packages/mcp/src/tool-specs.ts @@ -923,9 +923,10 @@ export const SHARED_TOOL_SPECS = { 'List the most recent pages (ordered by updatedAt, descending), ' + 'optionally scoped to a single space. Returns a bounded list (default ' + '50, max 100) — use search for lookups in large spaces. tree:true (with ' + - "spaceId) returns the space's full page hierarchy as a nested tree, but " + - 'is DEPRECATED — use getTree instead (leaner nodes, plus rootPageId / ' + - 'maxDepth).', + "spaceId) returns { tree, truncated } — the space's full page hierarchy " + + 'as a nested tree, plus a `truncated` flag that is true when the tree was ' + + 'capped and is INCOMPLETE — but is DEPRECATED, use getTree instead ' + + '(leaner nodes, plus rootPageId / maxDepth).', tier: 'core', catalogLine: "listPages — list recent pages (tree:true is deprecated; use getTree for the hierarchy).",